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

[full-ci] Enhancement: Add whitespace context-menu #8921

Merged
merged 21 commits into from
May 9, 2023

Conversation

lookacat
Copy link
Contributor

@lookacat lookacat commented Apr 26, 2023

Description

We've added a generic context-menu for right clicking on whitespace.
See #5861

Related Issue

Screenshots (if appropriate):

Screenshot (32)

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Technical debt
  • Tests

Checklist:

  • Code changes
  • Unit tests added
  • Acceptance tests added
  • Documentation ticket raised:

Open tasks:

@update-docs
Copy link

update-docs bot commented Apr 26, 2023

Thanks for opening this pull request! The maintainers of this repository would appreciate it if you would create a changelog item based on your changes.

@lookacat lookacat force-pushed the whitespace-contextmenu branch 2 times, most recently from 56f9404 to a4df0dd Compare April 26, 2023 14:20
@lookacat
Copy link
Contributor Author

Refactoring of the CreateAndUpload component into composables needed before this PR
#8938

@lookacat lookacat changed the title WIP Whitespace contextmenu [WIP] Whitespace contextmenu Apr 27, 2023
@lookacat lookacat changed the title [WIP] Whitespace contextmenu Whitespace contextmenu May 3, 2023
@lookacat lookacat changed the title Whitespace contextmenu [full-ci] Enhancement: Add whitespace context-menu May 4, 2023
@lookacat lookacat marked this pull request as ready for review May 4, 2023 09:41
@tbsbdr
Copy link
Contributor

tbsbdr commented May 4, 2023

cool one 👍

  • current clickable locations to get the menu is good imo ✅
  • A remark on Icon+wording: I'd suggest du directly show the folder icon (instead of the "+") and just call it "New Folder" (create new is semantically redundant)

image

@lookacat
Copy link
Contributor Author

lookacat commented May 5, 2023

@kulmann to the first point yes i can do that, the second point: Did you try it in the space root? in subfolders it should work, my assumption was there is no detail on the e.g. personal space root? Or did i miss something?

@lookacat lookacat requested a review from kulmann May 5, 2023 13:26
@kulmann
Copy link
Member

kulmann commented May 5, 2023

@kulmann to the first point yes i can do that, the second point: Did you try it in the space root? in subfolders it should work, my assumption was there is no detail on the e.g. personal space root? Or did i miss something?

ah. uh. I think both of my findings were bogus...

  • "Show Details" from whitespace context menu correctly clears the selection
  • "Show Details" from whitespace context menu correctly shows details...

For the personal space root it's a little bit confusing because the personal space doesn't have details. So that could be a tiny improvement: don't show the Show Details action in the whitespace context menu of the personal space root.

@lookacat
Copy link
Contributor Author

lookacat commented May 5, 2023

@kulmann alright shouldl be doable, the first point was right, i just fixed it ^^

Copy link
Member

Choose a reason for hiding this comment

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

Why did you decide to not use the ContextActionMenu.vue component? 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually you are right, was reverse engineering the context menu in order to understand it but didn't notice that I now can use the same component, wasn't clear if i would be able to use it at first

Copy link
Contributor Author

Choose a reason for hiding this comment

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

but will try to use it, shouldn't be much difference between those two besides the sections

Copy link
Contributor Author

Choose a reason for hiding this comment

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

arg, my assumption was wrong, can't get the ContextActionMenu to work at this point because _tippy is always undefined, would rename it to WhitespaceContextMenu for now :/

Copy link
Contributor Author

Choose a reason for hiding this comment

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

don't see how to quickly fix that, also there are some differences between those two component

@lookacat lookacat requested a review from JammingBen May 8, 2023 09:58
Copy link
Member

@kulmann kulmann left a comment

Choose a reason for hiding this comment

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

Found two more things. Also, what about unit tests, at least for opening the whitespace context menu (not for the actions inside it)?

@lookacat
Copy link
Contributor Author

lookacat commented May 8, 2023

@kulmann done

@lookacat lookacat requested a review from kulmann May 8, 2023 13:23
@sonarcloud
Copy link

sonarcloud bot commented May 8, 2023

SonarCloud Quality Gate failed.    Quality Gate failed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 11 Code Smells

46.9% 46.9% Coverage
0.0% 0.0% Duplication

Copy link
Member

@kulmann kulmann left a comment

Choose a reason for hiding this comment

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

Awesome stuff! Love the UX of this 😍

@kulmann kulmann merged commit 2960de8 into master May 9, 2023
@delete-merged-branch delete-merged-branch bot deleted the whitespace-contextmenu branch May 9, 2023 08:00
@micbar micbar mentioned this pull request Jul 24, 2023
68 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Open Context menu on click on whitespace
4 participants