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

Jump to bottom

Replace MenuBubble with a menu bar entry for links #2665

Merged
merged 3 commits into from
Oct 24, 2022
Merged

Replace MenuBubble with a menu bar entry for links #2665

merged 3 commits into from
Oct 24, 2022

Conversation

susnux
Copy link
Contributor

@susnux susnux commented Jul 4, 2022 •
edited
Loading

Summary

This replaces the popover MenuBubble used for inserting links with a menu entry in the menu bar.
This also allows to add links also to directories, previously only files were allowed as link targets.

Added cypress tests as well. Testing inserting links to files, to directories,
and also inserting links while nothing is selected to test if the correct file name is used.

Some the menu order is slightly changed to prevent the link menu to get hidden, like for the callouts
Actions must not get hidden in the submenu (nesting does not work for the menu).

Another feature introduced with this change is the ability to insert links to files and automatically use
the filename as the link text.

@susnux
Copy link
Contributor Author

susnux commented Jul 4, 2022

/compile

@susnux susnux mentioned this pull request Jul 4, 2022
Closed
@susnux susnux requested a review from vinicius73 July 4, 2022 12:52
@susnux susnux added enhancement New feature or request design Experience, interaction, interface, … 3. to review labels Jul 4, 2022
@susnux
Copy link
Contributor Author

susnux commented Jul 5, 2022

/compile

@susnux susnux requested a review from juliusknorr July 5, 2022 16:36
@susnux susnux force-pushed the feature/drop-menu-bubble branch 2 times, most recently from 59f8974 to c9152bd Compare July 6, 2022 17:09
@susnux
Copy link
Contributor Author

susnux commented Jul 6, 2022

/compile amend

@susnux
Copy link
Contributor Author

susnux commented Jul 6, 2022

Any feedback on this? :)

@mejo-
Copy link
Member

mejo- commented Jul 7, 2022

Thanks a lot for working on this @susnux! I'm much in favor of moving the link tools into the menu bar. But I'd say let's get #2611 merged first and then look at this one ☺️

@susnux
Copy link
Contributor Author

susnux commented Jul 7, 2022

Thanks a lot for working on this @susnux! I'm much in favor of moving the link tools into the menu bar. But I'd say let's get #2611 merged first and then look at this one relaxed

Thank you, no problem :)
I just would like to see this in one of the next releases as the menu bubble is quite annoying (I always have to scroll up for the bubble to be show even if I insert links at the bottom -.- )

@susnux susnux force-pushed the feature/drop-menu-bubble branch 2 times, most recently from 63580fe to 20617e7 Compare July 21, 2022 10:56
@susnux susnux force-pushed the feature/drop-menu-bubble branch 2 times, most recently from de15f59 to ef480a7 Compare September 6, 2022 12:15
@susnux
Copy link
Contributor Author

susnux commented Sep 6, 2022

This is currently blocked by #2842

@susnux susnux force-pushed the feature/drop-menu-bubble branch 2 times, most recently from 637dbe6 to 0139e9d Compare October 5, 2022 19:43
@susnux
Copy link
Contributor Author

susnux commented Oct 5, 2022

Not blocked anymore, so read to go 😊

@mejo-
Copy link
Member

mejo- commented Oct 5, 2022

@susnux thanks a lot! Would you mind adding a few screenshots to make it easier for our design team to review?

@susnux
Copy link
Contributor Author

susnux commented Oct 5, 2022

/compile amend

@susnux
Copy link
Contributor Author

susnux commented Oct 20, 2022

Thank you for the screenshots! Some feedback:

Thank you for your feedback! Fixed the first points, I will have a look at the last one soon.

@susnux
Copy link
Contributor Author

susnux commented Oct 20, 2022

ActionButton and ActionInput have different min-width. Fixed this by setting the min-width of button to the input.

Link menu
Link menu insert
Link menu edit

mejo-
mejo- approved these changes Oct 20, 2022
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.

Tested it and it's working great. Code also looking good 👌 Again, thanks a lot @susnux.

src/components/Menu/ActionInsertLink.vue
// Avoid issues when parsing urls later on in markdown that might be entered in an invalid format (e.g. "mailto: example@example.com")
const href = url.replaceAll(' ', '%20')
const chain = this.$editor.chain()
// Check if any text is selected, if not insert the lunk using the given text property
Copy link
Member

Choose a reason for hiding this comment

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

Minor typo 😉 s/lunk/link

@mejo-
Copy link
Member

mejo- commented Oct 20, 2022

/compile amend

@mejo-
Copy link
Member

mejo- commented Oct 20, 2022

The updated anchor link cypress test fails though. Could you take a look @susnux? Afterwards good to go from my side 👍

@mejo- mejo- dismissed jancborchardt’s stale review October 20, 2022 20:20

all requested changes implemented

@susnux
Copy link
Contributor Author

susnux commented Oct 21, 2022

The updated anchor link cypress test fails though. Could you take a look @susnux? Afterwards good to go from my side +1

Should be fixed now, works locally waiting for CI to succeed :)

@susnux
Copy link
Contributor Author

susnux commented Oct 21, 2022

/compile amend

@mejo-
Copy link
Member

mejo- commented Oct 21, 2022

/compile

@mejo-
Copy link
Member

mejo- commented Oct 21, 2022

For some reason, the node build + porcelain check CI job always reports a diff 🙁 Will have to look into it next week.

@susnux @mejo-
Replace MenuBubble with menu bar entry for links
e286194 
This replaces the popover MenuBubble used for inserting
links with a menu entry in the menu bar, fixing #2392.
This also allows to add links also to directories,
previously only files were allowed as link targets, fixing #2162.

Signed-off-by: Ferdinand Thiessen <rpm@fthiessen.de>
@susnux @mejo-
Remember last directory for linking to files to directories
69d4b5a 
Signed-off-by: Ferdinand Thiessen <rpm@fthiessen.de>
@mejo-
Copy link
Member

mejo- commented Oct 24, 2022

/compile

@mejo-
Copy link
Member

mejo- commented Oct 24, 2022

/compile amend

@mejo- @nextcloud-command
Compile assets
3e1ffbd 
Signed-off-by: Jonas <jonas@freesources.org>
Signed-off-by: nextcloud-command <nextcloud-command@users.noreply.github.com>
@mejo- mejo- merged commit 691cbe4 into master Oct 24, 2022
@delete-merged-branch delete-merged-branch bot deleted the feature/drop-menu-bubble branch October 24, 2022 09:06
@susnux
Copy link
Contributor Author

susnux commented Oct 25, 2022

Not sure if this can be backported to NC25, but would it be acceptable to backport at least the "link to folders" part?

@mejo-
Copy link
Member

mejo- commented Oct 25, 2022

Usually we don't backport features to stable releases except if there's a strong reason to do so.

@vinicius73 vinicius73 mentioned this pull request Oct 27, 2022
Merged
@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

@vinicius73 vinicius73 Awaiting requested review from vinicius73

@juliusknorr juliusknorr Awaiting requested review from juliusknorr

@jancborchardt jancborchardt Awaiting requested review from jancborchardt

Assignees
No one assigned
Labels
3. to review design Experience, interaction, interface, … enhancement New feature or request
Projects
None yet
Milestone
Nextcloud 26
4 participants
@susnux @mejo- @jancborchardt @juliusknorr