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

Simplify mutation menu #1775

Merged
merged 4 commits into from
May 22, 2024
Merged

Simplify mutation menu #1775

merged 4 commits into from
May 22, 2024

Conversation

MetRonnie
Copy link
Member

@MetRonnie MetRonnie commented May 2, 2024

By figuring out how to use VMenu better I was able to drop a fair amount of cruft.

We were originally using VMenu but through various attempts to get it to behave the way we want (e.g. #927, #1269) I ended up re-implementing some of the behaviour manually (e.g. click-outside behaviour).

It turns out the key to using VMenu and getting it to behave the way we want is not too difficult:

  • :target prop - this allows Vuetify to take care of positioning relative to the c-data-interactive element that was clicked on
  • :key prop - this allows the menu to re-open when clicking between c-data-interactive elements by using a unique identifier for each element (so Vue knows a different element has been clicked)

Update: also renamed the v-cylc-object directive to v-command-menu as that is more descriptive IMO

Check List

  • I have read CONTRIBUTING.md and added my name as a Code Contributor.
  • Contains logically grouped changes (else tidy your branch by rebase).
  • Does not contain off-topic changes (use other PRs for other changes).
  • Tests are included
  • CHANGES.md entry not needed as invisible to users

@MetRonnie MetRonnie added this to the 2.6.0 milestone May 2, 2024
@MetRonnie MetRonnie self-assigned this May 2, 2024
@MetRonnie MetRonnie marked this pull request as draft May 3, 2024 09:36
@MetRonnie MetRonnie mentioned this pull request May 7, 2024
8 tasks
@MetRonnie MetRonnie marked this pull request as ready for review May 7, 2024 12:17
@MetRonnie MetRonnie modified the milestones: 2.6.0, 2.5.0 May 7, 2024
Copy link
Member

@oliver-sanders oliver-sanders left a comment

Choose a reason for hiding this comment

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

Looks good to me.

Spotted one issue in the graph view where opening the menu on different jobs didn't behave as elsewhere. Haven't checked this against master, however, it appears to be fixed with #1780

@MetRonnie
Copy link
Member Author

MetRonnie commented May 8, 2024

Latest change should make it immune to that kind of thing

@oliver-sanders
Copy link
Member

Fixed.

@MetRonnie

This comment was marked as resolved.

@MetRonnie
Copy link
Member Author

MetRonnie commented May 13, 2024

I've also renamed the v-cylc-object directive to v-command-menu as that is more descriptive IMO

Copy link
Member

@oliver-sanders oliver-sanders left a comment

Choose a reason for hiding this comment

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

I found one "quirk" when a task disappears from the n-window and is removed from the view, any open menu will jump to the top-left of the screen (because the target node has been removed).

Would be beneficial if we could disable this behaviour, but the menu remains functional so not a biggie.

:key="node.id"
v-click-outside="{ handler: onClickOutside, closeConditional: () => !dialog }"
class="c-mutation-menu elevation-10 overflow-y-auto"
max-height="90vh"
Copy link
Member

Choose a reason for hiding this comment

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

Tested with small windows, the default max-height behaviour appears to be sufficient.

@MetRonnie MetRonnie merged commit 1ecfe08 into cylc:master May 22, 2024
8 checks passed
@MetRonnie MetRonnie removed this from the 2.6.0 milestone May 22, 2024
@MetRonnie MetRonnie added this to the 2.5.0 milestone May 22, 2024
@MetRonnie MetRonnie deleted the mutation-menu branch May 22, 2024 17:01
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.

None yet

3 participants