Smart url parsing #2141
Smart url parsing #2141
Conversation
Trombach
requested review from
dessalines,
SleeplessOne1917,
alectrocute and
jsit
as code owners
This is the better approach in my opinion. I'll wait for others to chime in on how it should be done before checking out the code. |
|
I agree with sleepless, it'd be better to detect selected text first, then a paste event can turn it into a markdown link. |
| i.setState({ | ||
| content: `${ | ||
| i.state.content ? i.state.content.substring(0, selectionStart) : "" | ||
| }[${selectedText}](${url.toString()})${ | ||
| i.state.content ? i.state.content.substring(selectionEnd) : "" | ||
| }`, | ||
| }); |
There was a problem hiding this comment.
I'm pretty sure inferno has similar issues to react when trying to set state using a previous value of that state. Try this:
| i.setState({ | |
| content: `${ | |
| i.state.content ? i.state.content.substring(0, selectionStart) : "" | |
| }[${selectedText}](${url.toString()})${ | |
| i.state.content ? i.state.content.substring(selectionEnd) : "" | |
| }`, | |
| }); | |
| i.setState(({content}) => ({ | |
| content: `${content?.substring(0, selectionStart) ?? ""}[${selectedText}](${url.toString()})${content?.substring(selectionEnd) ?? ""}` | |
| })); |
You will probably need to format this with prettier still.
| if (textarea instanceof HTMLTextAreaElement) { | ||
| event.preventDefault(); | ||
| const { selectionStart, selectionEnd } = textarea; | ||
| const selectedText = i.getSelectedText() || url.toString(); |
There was a problem hiding this comment.
If the return value of getSelectedText can be null or undefined, it is slightly clearer to use ?? instead of ||.
There was a problem hiding this comment.
I actually used || specifically since getSelectedText can't return null or undefined but could return "". I wanted to fallback to the url in that case, but since the consensus seems to be that the smart formatting should not be applied if no text is selected this fallback will become obsolete.
|
@SleeplessOne1917 @dessalines Thanks for the initial feedback. I have updated the PR and brought the behaviour closer to what Github does. I have updated the video as well |
Trombach
force-pushed
the
smart-url-paste
branch
from
27c4e38 to
6eb6533
Compare
| } | ||
| } | ||
|
|
||
| isValidUrl(value: string) { |
There was a problem hiding this comment.
This function should either be renamed or inlined. Seeing a url returned from what the name makes seem should be a boolean threw me through a loop for a second.
There was a problem hiding this comment.
Also, make sure the return type is explicitly named: : bool
Also, double check using grep to see if we already have this function... seems like we should.
| const url = i.isValidUrl(event.clipboardData.getData("text")); | ||
| if (url) { | ||
| i.handleUrlPaste(url, i, event); | ||
| return; |
There was a problem hiding this comment.
This return statement is unnecessary since it's at the end of the function anyway.
| const { selectionStart, selectionEnd } = textarea; | ||
|
|
||
| // if no selection, just insert url | ||
| if (selectionStart === selectionEnd) return; |
There was a problem hiding this comment.
How is it doing anything here?
There was a problem hiding this comment.
The comment explains it. If no text is selected (i.e. the curser is in one position and therefore selectionStart === selectionEnd) we don't want to apply formatting
There was a problem hiding this comment.
Gotcha, it has to do with event.preventDefault() not being called.
| } | ||
| } | ||
|
|
||
| isValidUrl(value: string) { |
There was a problem hiding this comment.
Also, make sure the return type is explicitly named: : bool
Also, double check using grep to see if we already have this function... seems like we should.
| } | ||
|
|
||
| isValidUrl(value: string) { | ||
| if (!value) return false; |
There was a problem hiding this comment.
This isn't necessary, you aren't passing in string | undefined, so we can assume its there.
There was a problem hiding this comment.
Due to the beauty of javascript and "" being falsy, it does something :) It actually prevents a constructor call to URL if you pass in an empty string which would never be a valid URL. I'm moving to the validUrl helper function, so it won't be relevant.
| const { selectionStart, selectionEnd } = textarea; | ||
|
|
||
| // if no selection, just insert url | ||
| if (selectionStart === selectionEnd) return; |
There was a problem hiding this comment.
Gotcha, it has to do with event.preventDefault() not being called.
Description
This PR resolves #2116
Update: The behaviour is now a bit more similar to how Github handles this. I have also updated the video to show the new behaviour
Adds logic to detect if a paste event contains a URL. If true the URL will be formatted with the appropriate markdown. Open to suggestions on how to handle text selection after the paste event. The current implementation is demonstrated in the videos below.
It might also be better to implement this in the same way github does this, only adding formatting if text is already selected and otherwise just pasting the plain link, which might make things easier to read in the editor if no alternative link text is desired.
Screenshots
Before
Screencast.from.2023-09-24.17-30-03.webm
After
Screencast.from.2023-09-26.21-59-39.webm