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

Replace MenuBubble with a menu bar entry for links #2519

Closed
wants to merge 3 commits into from
Closed

Replace MenuBubble with a menu bar entry for links #2519

wants to merge 3 commits into from

Conversation

susnux
Copy link
Contributor

@susnux susnux commented Jun 17, 2022

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.

@vinicius73
Copy link
Member

I'm trying to test it locally, but I'm having errors to build @susnux

image

@susnux
Copy link
Contributor Author

susnux commented Jun 23, 2022

I'm trying to test it locally, but I'm having errors to build @susnux

That is not a issue of this PR, but a problem within the current package.json.

5b5d6cf added "stylelint-config-recommended-scss": "^6.0.0" but this conflicts with "@nextcloud/stylelint-config" (which requires "^5.0.2").
So this leads to a broken state of the node_modules, you can fix this by set "stylelint-config-recommended-scss": "^5.0.2" in the package.json and rerun npm install.

@vinicius73
Copy link
Member

I'm not having this issue in master @susnux, but I will check, thanks.

@susnux
Copy link
Contributor Author

susnux commented Jun 23, 2022

I'm not having this issue in master @susnux, but I will check, thanks.

I think this is triggered by using the ActionInput component which (might) use @nextcloud/calendar-js and then node is searching for the ical.js and somehow encounters the problem with that mismatching versions. As for me npm install fails with:

grafik

@vinicius73
Copy link
Member

vinicius73 commented Jun 23, 2022

Looks like a "npm version problem"
We recommend the usage of Node v14 + NPM v7
But, NPM v7 is not shipped with Node v14...

Try to use npm ci and npm run build commands @susnux

@susnux
Copy link
Contributor Author

susnux commented Jun 23, 2022

Looks like a "npm version problem" We recommend the usage of Node v14 + NPM v7 But, NPM v7 is not shipped with Node v14...

@vinicius73 Normally I use node16 + npm8 (as node14 is EOL and my distribution does not ship it anymore), but for this I tried also node14+npm7: Result is the same there is a package version mismatch (same error as above).


This issue is related to the ActionInput component used in the PR, as it uses the DatetimePicker which uses @nextcloud/calendar-js. The problem is @nextcloud/calendar-js has a peerDependency on ical.js.

Now normally npm installs unsatisfied peerDependencies, but I think because of the version issue (stylelint-config-recommended-scss), it skips the needed installation of that package.

@skjnldsv
Copy link
Member

as node14 is EOL

Node 14 is still maintained as last LTS for 10 more months
https://endoflife.date/nodejs

@vinicius73
Copy link
Member

/rebase

susnux added 2 commits July 1, 2022 08:51
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>
Testing inserting links to files, to directories,
and also inserting links while nothing is selected
to test if the correct file name is used.

Signed-off-by: Ferdinand Thiessen <rpm@fthiessen.de>
@susnux
Copy link
Contributor Author

susnux commented Jul 1, 2022

@vinicius73

/rebase

Rebased manually there were some conflicts with the cypress part.

Signed-off-by: Ferdinand Thiessen <rpm@fthiessen.de>
@susnux
Copy link
Contributor Author

susnux commented Jul 4, 2022

I moved the PR to #2665, so the assets can be compiled (and the node CI test succeeds), as suggested here #2628 (comment)

@susnux susnux closed this Jul 4, 2022
@susnux susnux deleted the feature/drop-menu-bubble branch July 4, 2022 12:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants