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.

Sign up for GitHub

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

Smart url parsing #2141

Merged
merged 6 commits into from
Sep 28, 2023
Merged

Smart url parsing #2141

Changes from 3 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
2a2c8c9
handle url paste
Trombach Sep 19, 2023
ce7949d
change selection range after paste
Trombach Sep 24, 2023
6eb6533
use default paste behaviour if no text is selected
Trombach Sep 26, 2023
eec514d
Merge branch 'main' into smart-url-paste
SleeplessOne1917 Sep 28, 2023
de5dfb7
change to `validUrl` helper function
Trombach Sep 28, 2023
d830e69
Merge branch 'main' into smart-url-paste
dessalines Sep 28, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
57 changes: 55 additions & 2 deletions src/shared/components/common/markdown-textarea.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -233,7 +233,7 @@ export class MarkdownTextArea extends Component<
)}
value={this.state.content}
onInput={linkEvent(this, this.handleContentChange)}
onPaste={linkEvent(this, this.handleImageUploadPaste)}
onPaste={linkEvent(this, this.handlePaste)}
onKeyDown={linkEvent(this, this.handleKeyBinds)}
required
disabled={this.isDisabled}
Expand Down Expand Up @@ -374,10 +374,63 @@ export class MarkdownTextArea extends Component<
autosize.update(textarea);
}

handleImageUploadPaste(i: MarkdownTextArea, event: any) {
handlePaste(i: MarkdownTextArea, event: ClipboardEvent) {
if (!event.clipboardData) return;

// check clipboard files
const image = event.clipboardData.files[0];
if (image) {
i.handleImageUpload(i, image);
return;
}

// check clipboard url
const url = i.isValidUrl(event.clipboardData.getData("text"));
if (url) {
i.handleUrlPaste(url, i, event);
return;
Copy link
Member

Choose a reason for hiding this comment

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

This return statement is unnecessary since it's at the end of the function anyway.

}
}

handleUrlPaste(url: URL, i: MarkdownTextArea, event: ClipboardEvent) {
// query textarea element
const textarea = document.getElementById(i.id);

if (textarea instanceof HTMLTextAreaElement) {
const { selectionStart, selectionEnd } = textarea;

// if no selection, just insert url
if (selectionStart === selectionEnd) return;
Copy link
Member

Choose a reason for hiding this comment

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

How is it doing anything here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

Gotcha, it has to do with event.preventDefault() not being called.


event.preventDefault();
const selectedText = i.getSelectedText();

// update textarea content
i.setState(({ content }) => ({
content: `${
content?.substring(0, selectionStart) ?? ""
}[${selectedText}](${url.toString()})${
content?.substring(selectionEnd) ?? ""
}`,
}));
i.contentChange();

// shift selection 1 to the right
textarea.setSelectionRange(
selectionStart + 1,
selectionStart + 1 + selectedText.length,
);
}
}

isValidUrl(value: string) {
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

@dessalines dessalines Sep 27, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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.

if (!value) return false;
Copy link
Member

Choose a reason for hiding this comment

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

This isn't necessary, you aren't passing in string | undefined, so we can assume its there.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.


try {
const url = new URL(value);
return url;
} catch {
return false;
}
}

Expand Down