Skip to content

Link picker#3700

Merged
juliusknorr merged 12 commits intomainfrom
enh/link-picker
Jan 30, 2023
Merged

Link picker#3700
juliusknorr merged 12 commits intomainfrom
enh/link-picker

Conversation

@juliusknorr
Copy link
Member

@juliusknorr juliusknorr commented Jan 26, 2023 •
edited
Loading

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 •
edited
Loading

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 •
edited by juliusknorr
Loading

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

@juliusknorr
Copy link
Member Author

juliusknorr commented Jan 26, 2023

@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.

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

julien-nc commented Jan 27, 2023

@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 commented Jan 27, 2023

@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

julien-nc commented Jan 27, 2023

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

juliusknorr and others added 10 commits January 30, 2023 14:35
@juliusknorr
refactor(suggestion): Refactor existing suggestions to reuse logic
603096b 
Signed-off-by: Julius Härtl <jus@bitgrid.net>
@julien-nc @juliusknorr
use vue-richtext helper functions to call the link picker
1a39895 
Signed-off-by: Julien Veyssier <julien-nc@posteo.net>
@juliusknorr
fix(Suggestion): Close link picker tippy if selected
7c72748 
Signed-off-by: Julius Härtl <jus@bitgrid.net>
@juliusknorr
fix(LinkPicker): Show icons
4a49052 
Signed-off-by: Julius Härtl <jus@bitgrid.net>
@juliusknorr
fix(LinkPicker): Make sure that spaces after link paragraphs are ignored
9a05d97 
Signed-off-by: Julius Härtl <jus@bitgrid.net>
@juliusknorr
tests(Mentions): Fix cypress selector for mentions test
26c2993 
Signed-off-by: Julius Härtl <jus@bitgrid.net>
@julien-nc @juliusknorr
adjust link picker suggestion style
e02a2a1 
Signed-off-by: Julien Veyssier <julien-nc@posteo.net>
@julien-nc @juliusknorr
allow larger suggestion list
a116634 
Signed-off-by: Julien Veyssier <julien-nc@posteo.net>
@julien-nc @juliusknorr
add menu action to open link picker
6cda803 
Signed-off-by: Julien Veyssier <julien-nc@posteo.net>
@juliusknorr
chore(dependencies): Bump @nextcloud/vue-richtext
932a2dc 
Signed-off-by: Julius Härtl <jus@bitgrid.net>
@juliusknorr
Copy link
Member Author

juliusknorr commented Jan 30, 2023

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

@juliusknorr juliusknorr mentioned this pull request Jan 30, 2023
Merged
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.

julien-nc
julien-nc reviewed Jan 30, 2023
View reviewed changes
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.

@juliusknorr
fix: Properly destroy tippy on escape
3ac47b9 
Signed-off-by: Julius Härtl <jus@bitgrid.net>
@juliusknorr
Copy link
Member Author

juliusknorr commented Jan 30, 2023

Pushed a fix that applies for all autocomplete components now.

mejo-
mejo- approved these changes Jan 30, 2023
View reviewed changes
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
julien-nc
julien-nc approved these changes Jan 30, 2023
View reviewed changes
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

julien-nc commented Jan 30, 2023

/compile

@nextcloud-command
Compile assets
82036c3 
Signed-off-by: nextcloud-command <nextcloud-command@users.noreply.github.com>
@juliusknorr juliusknorr 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
This was referenced Feb 2, 2023
Closed
Closed
@blizzz blizzz mentioned this pull request Feb 9, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mejo- mejo- mejo- approved these changes

@julien-nc julien-nc julien-nc approved these changes

@max-nextcloud max-nextcloud Awaiting requested review from max-nextcloud max-nextcloud is a code owner

Assignees

No one assigned

Labels

3. to review enhancement New feature or request

Projects

None yet

Milestone

Nextcloud 26

Development

Successfully merging this pull request may close these issues.

4 participants

@juliusknorr @julien-nc @mejo- @nextcloud-command