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

Unify linkify usage, move to linkify-string #2275

Merged
merged 1 commit into from
Sep 24, 2021

Conversation

raimund-schluessler
Copy link
Contributor

@raimund-schluessler raimund-schluessler commented Sep 22, 2021

This unifies the linkify-string usage (we had this function in two places) and switches to the linkify-string package which replaces linkifyjs/string since the latest major release of linkifyjs.

Supersedes #2257.

Closes #1488.

@raimund-schluessler raimund-schluessler added 3. to review Waiting for reviews dependencies Pull requests that update a dependency file feature: rich-contenteditable Related to the rich-contenteditable components labels Sep 22, 2021
@raimund-schluessler raimund-schluessler added this to the 4.1.2 milestone Sep 22, 2021
@nickvergessen
Copy link
Contributor

Ref #1678

We have to check if Safari still works with this

@raimund-schluessler
Copy link
Contributor Author

raimund-schluessler commented Sep 23, 2021

Ref #1678

We have to check if Safari still works with this

At least for Safari on iOS it still works properly.

Also see nextcloud/calendar#3490 where the changes are present already.

The previously used linkifyjs/string just moved to its own package linkify-string. So it's the same Code that is used, just without the rest of linkify. This might even reduce the bundle size (need to check).

@raimund-schluessler
Copy link
Contributor Author

I checked the bundle size, and it stays quite exactly the same. Webpack only bundled linkifyjs/string before and now only bundles linkify-string and both are the same size (and don't include jquery). So, #1488 shouldn't be an issue anyway anymore.

I fixed a small issue with linkification in the AppSidebar title, because now the server a.external style applies, which introduced a left/right margin for links. I overwrote this to not change the AppSidebar style.

I think this PR should be good now (maybe just test, that it really works with Safari on the Desktop).

st3iny
st3iny previously requested changes Sep 24, 2021
Copy link
Contributor

@st3iny st3iny left a comment

Choose a reason for hiding this comment

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

Looks good. I left some minor remarks regarding the documentation.

src/utils/Linkify.js Outdated Show resolved Hide resolved
Copy link
Contributor

@st3iny st3iny left a comment

Choose a reason for hiding this comment

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

LGTM! Please squash before merging :)

@raimund-schluessler raimund-schluessler modified the milestones: 4.1.2, 4.2.0 Sep 24, 2021
@raimund-schluessler raimund-schluessler merged commit ee794f3 into master Sep 24, 2021
@raimund-schluessler raimund-schluessler deleted the fix/linkify-string branch September 24, 2021 10:47
@raimund-schluessler
Copy link
Contributor Author

/backport to stable4

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3. to review Waiting for reviews dependencies Pull requests that update a dependency file feature: rich-contenteditable Related to the rich-contenteditable components
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Move away from linkifyjs
5 participants