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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Link picker #3700

Merged
merged 12 commits into from Jan 30, 2023
Merged

Link picker #3700

merged 12 commits into from Jan 30, 2023

Conversation

juliushaertl
Copy link
Member

@juliushaertl juliushaertl commented Jan 26, 2023

Signed-off-by: Julius H盲rtl jus@bitgrid.net

馃摑 Summary

  • Refactor existing suggestion extensions to make code reusable
  • Implement a link picker plugiun
    • Prepare extension
    • Fetch link provider list from the backend
    • Trigger link picker on provider
    • Make sure that result gets inserted
    • Adapt mention cypress tests to new component layout

Depends on

馃弫 Checklist

  • Code is properly formatted (npm run lint / npm run stylelint / composer run cs:check)
  • Sign-off message is added to all commits
  • Tests (unit, integration and/or end-to-end) passing and the changes are covered with tests
  • Documentation (README or documentation) has been updated or is not required

@cypress
Copy link

cypress bot commented Jan 26, 2023

Passing run #8377 鈫楋笌

0 116 0 0 Flakiness 0

Details:

Link picker
Project: Text Commit: 82036c3145
Status: Passed Duration: 03:59 馃挕
Started: Jan 30, 2023 4:35 PM Ended: Jan 30, 2023 4:39 PM

This comment has been generated by cypress-bot as a result of this project's GitHub integration settings.

@julien-nc
Copy link
Member

julien-nc commented Jan 26, 2023

Awesome!

I pushed a commit adding the glue to use the link picker in your suggestion plugin.

Text needs to be linked with @nextcloud/vue-richtext 's "enh/noid/reference-picker" branch.

Remaining issues:

  • Suggestions (provider list) still visible when the link picker modal is displayed
  • If we insert the link with a trailing space, rendering is not triggered. I guess we could insert a link node instead of the raw text (problem: we might get some text meant to be inserted but not as a link).
  • When opening a "search" provider, there is a focus loop issue slowing down the browser for a few seconds. Seems to be a focus trap issue.
  • We need to figure out a way to display the provider icon (from its URL) in the suggestions

@juliushaertl
Copy link
Member Author

@julien-nc Pushed a few very minor fixes for the issues you found. The focus trap issue I could not reproduce so I assume it is a side effect of another app that you have enabled on your setup.

@juliushaertl juliushaertl marked this pull request as ready for review January 26, 2023 21:36
@juliushaertl juliushaertl added 3. to review enhancement New feature or request labels Jan 26, 2023
@juliushaertl juliushaertl added this to the Nextcloud 26 milestone Jan 26, 2023
@julien-nc
Copy link
Member

@juliushaertl Nice!
I've pushed a few adjustments:

  • make the provider icons smaller to make it consistent with those in RichContentEditable (which size was set to be consistent with the emojis)
  • increase suggestion list max-width to 400px because provider names can be long

Are you ok with those changes?

@julien-nc
Copy link
Member

@julien-nc Pushed a few very minor fixes for the issues you found. The focus trap issue I could not reproduce so I assume it is a side effect of another app that you have enabled on your setup.

After frantically updating @nextcloud/vue in all my apps I realized I hadn't pull viewer for a long time...all good for the focus trap now.

@julien-nc
Copy link
Member

"Open link picker" action is there. The icon might not be the best one.

juliushaertl and others added 10 commits January 30, 2023 14:35
Signed-off-by: Julius H盲rtl <jus@bitgrid.net>
Signed-off-by: Julien Veyssier <julien-nc@posteo.net>
Signed-off-by: Julius H盲rtl <jus@bitgrid.net>
Signed-off-by: Julius H盲rtl <jus@bitgrid.net>
Signed-off-by: Julius H盲rtl <jus@bitgrid.net>
Signed-off-by: Julius H盲rtl <jus@bitgrid.net>
Signed-off-by: Julien Veyssier <julien-nc@posteo.net>
Signed-off-by: Julien Veyssier <julien-nc@posteo.net>
Signed-off-by: Julien Veyssier <julien-nc@posteo.net>
Signed-off-by: Julius H盲rtl <jus@bitgrid.net>
@juliushaertl
Copy link
Member Author

Ready for review and good to get in from my side 馃憤

Let's file follow up tickets for:

  • Avoid to close text on esc key press
  • Cypress tests

@juliushaertl juliushaertl mentioned this pull request Jan 30, 2023
21 tasks
@mejo-
Copy link
Member

mejo- commented Jan 30, 2023

Very nice and works well 鉂わ笍

Unfortunately it brings back a bug that I had fixed in 6eb324f before: when opening the emoji picker, closing it with Esc and hitting enter, the emoji that was selected last still gets inserted. That's particularly annoying in languages like french, where single colons are often written.

The same bug exists with mentions (@), but there it's less annoying because single @ is not used as often.

Copy link
Member

@julien-nc julien-nc left a comment

Choose a reason for hiding this comment

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

Awesome. The Text operating system is on its way.

Can confirm the bug mentioned by @mejo- for all users, emojis and link providers.

Signed-off-by: Julius H盲rtl <jus@bitgrid.net>
@juliushaertl
Copy link
Member Author

Pushed a fix that applies for all autocomplete components now.

Copy link
Member

@mejo- mejo- left a comment

Choose a reason for hiding this comment

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

Awesome. Works great and the latest fix by @juliushaertl also works 鉂わ笍

@julien-nc julien-nc self-requested a review January 30, 2023 16:25
Copy link
Member

@julien-nc julien-nc left a comment

Choose a reason for hiding this comment

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

馃憤

@julien-nc
Copy link
Member

/compile

Signed-off-by: nextcloud-command <nextcloud-command@users.noreply.github.com>
@juliushaertl juliushaertl merged commit 84b65e1 into main Jan 30, 2023
@delete-merged-branch delete-merged-branch bot deleted the enh/link-picker branch January 30, 2023 16:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3. to review enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants