Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(#22): Setup markdown editor #40

Merged
merged 11 commits into from
Apr 18, 2021

Conversation

ddsuhaimi
Copy link
Contributor

No description provided.

package.json Outdated Show resolved Hide resolved
client/src/services/injectElipsis.js Outdated Show resolved Hide resolved
client/src/components/RichTextEditor/draft.css Outdated Show resolved Hide resolved
@Mayank0255
Copy link
Owner

@ddsuhaimi I have requested some small changes on the PR, other than that great work.

@ddsuhaimi
Copy link
Contributor Author

@Mayank0255
Updated with the requested changes

@Mayank0255
Copy link
Owner

@ddsuhaimi

Great job man.

I just checked it out and it works fine for the functionalities given but you have forgotten to add code and hyperlink functionality to it. While adding the code one don't forget to look at the ui of original stackoverflow clone.

And one doubt I had is that what is htmlSubstring used for with injectEllipsis?

@Mayank0255
Copy link
Owner

@ddsuhaimi One last thing that don't forget to remove the monospace feature as it's not required

@Mayank0255 Mayank0255 linked an issue Apr 4, 2021 that may be closed by this pull request
@ddsuhaimi
Copy link
Contributor Author

ddsuhaimi commented Apr 4, 2021

@Mayank0255
For htmlSubstring and injectEllipsis, I used it for post item in the homepage. Normally, you substring the body to show only first 200 characters right?
Since what we have right now is a string of html, I can't simply do body.substring(200), so htmlSubstring comes to play. What it does is that it will extract the first 200 character without touching the html tag (so we wont lose any formatting). I then created injectEllipsis to inject ellipsis at the end of that substring. If I dont do that, when it happen to cut off in the middle of the word, it looks weird.

For the other 2, the converter library still having an open issues for code block (HubSpot/draft-convert#182) (you can uncomment the code block, it works but having conversion issues) and I'll see how to implement the link since it doesn't come by default in draft.js

@Mayank0255
Copy link
Owner

For htmlSubstring and injectEllipsis, I used it for post item in the homepage. Normally, you substring the body to show only first 200 characters right?

Yes, good work 👍🏽

I'll see how to implement the link since it doesn't come by default in draft.js

Sure, do that please and work on the code block thing and removing the monospace too while you are at it.

@Mayank0255
Copy link
Owner

@ddsuhaimi

What's the progress on this?

@ddsuhaimi
Copy link
Contributor Author

@ddsuhaimi

Sorry, it is probably gonna take a while. I've been kinda busy lately. I'll develop this in the weekend.

@Mayank0255
Copy link
Owner

@ddsuhaimi Sure

@Mayank0255
Copy link
Owner

@ddsuhaimi Are you facing any kind of problem somewhere?

@ddsuhaimi
Copy link
Contributor Author

@Mayank0255

Hi, sorry it took a while. Turns out, the issue is on 'draft-convert' library itself. It just can't convert code block to its corresponding html (and it is actually an open issue in its repo and still hasn't been resolved). So, there is pretty much no use of having code block in the editor when we can't convert it. This is also the case for link.

@Mayank0255
Copy link
Owner

@ddsuhaimi

Okay, I see

Here's an answer for draftjs to html conversion by the author himself - Here

In which the author has recommended to use the libraries listed over there as it's officially built by them and for a running example you can see this and the repository to which is listed here

In the demo shown code block is specifically implemented also. The link to the package repo is here

I hope this solves the issue and if not then let me know.

I know this can be tiring to find out the solution but till know you have done a marvelous job so it won't hurt to finish it off perfectly too. It might seem a big task of replacing the draft-convert with this but it would take hardly and hour after figuring out the approach and for which I am here.

P.S. - I am thinking of integrating docusaurus to the backend in which along with the API, a blog page would also be there in which news would come out referring the contributors and congratulating them and explaining about the contribution to the project. This is just to tell you that the contribution won't be in vain.

@ddsuhaimi
Copy link
Contributor Author

@Mayank0255

so is it okay if instead of using pure draft-js with draft-convert, we use react-rte? which i think, under the hood, is draft-js?

@Mayank0255
Copy link
Owner

@ddsuhaimi Sure, go ahead

As long as it satisfies our needs, its fine

Copy link
Owner

@Mayank0255 Mayank0255 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ddsuhaimi

The code looks pretty much fine, so for better review and my convenience could you add two screenshots?

  1. of the editor with information written in it while applying most of the essential features, like bold, italics, underline, code block, lists, hyperlinks, etc.
  2. of the rendered output of the same in the Post page of the particular post you added

Would that be fine with you to attach?

@ddsuhaimi
Copy link
Contributor Author

@Mayank0255
Sure thing.

image
image

@Mayank0255
Copy link
Owner

@ddsuhaimi Good job man!!

Thank you for working patiently with me 😄

Looking forward to more of the contributions from your side

@Mayank0255 Mayank0255 changed the title Setup markdown editor feat(#22): Setup markdown editor Apr 18, 2021
@Mayank0255 Mayank0255 merged commit d3ecb29 into Mayank0255:master Apr 18, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Frontend]: Setup Markdown Editor
2 participants