Skip to content

fix(frontend): prevent multiple menu opening #5649

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

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

Guilhem-lm
Copy link
Contributor

@Guilhem-lm Guilhem-lm commented Apr 21, 2025

Important

Prevent multiple menu openings by using unique modalId and excluding these IDs from click outside events.

  • Behavior:
    • Prevents multiple menu openings by introducing modalId in AppButton.svelte and AppMenu.svelte.
    • Excludes modalId from click outside events in Menu.svelte to prevent closing other menus.
  • Components:
    • Adds modalId and modalWrapperClass props to ConfirmationModal.svelte.
    • Calls onSuccess callback in AppButton.svelte after setting list inputs.
  • Misc:
    • Uses crypto.randomUUID() to generate unique modalId in AppMenu.svelte.
    • Adjusts getMenuElements() in Menu.svelte to include elements with modalId.

This description was created by Ellipsis for 04f7b03. You can customize this summary. It will automatically update as commits are pushed.

Copy link

cloudflare-workers-and-pages bot commented Apr 21, 2025

Deploying windmill with  Cloudflare Pages  Cloudflare Pages

Latest commit: 04f7b03
Status: ✅  Deploy successful!
Preview URL: https://5bc8abcd.windmill.pages.dev
Branch Preview URL: https://glm-fix-frontend-prevent-mul.windmill.pages.dev

View logs

@Guilhem-lm Guilhem-lm marked this pull request as ready for review May 13, 2025 08:54
Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

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

Important

Looks good to me! 👍

Reviewed everything up to 57bfa27 in 1 minute and 17 seconds. Click for details.
  • Reviewed 57 lines of code in 1 files
  • Skipped 0 files when reviewing.
  • Skipped posting 3 draft comments. View those below.
  • Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. frontend/src/lib/components/meltComponents/Menu.svelte:20
  • Draft comment:
    Refactored the destructuring of createMenu's return value to remove duplication. This improves clarity—ensure the melt directive still applies the correct attributes for ID usage.
  • Reason this comment was not posted:
    Confidence changes required: 0% <= threshold 50% None
2. frontend/src/lib/components/meltComponents/Menu.svelte:42
  • Draft comment:
    The getMenuElements function now queries using [data-melt-menu-id="$menuId"], ensuring only elements for the current menu instance are selected. Confirm that the melt directive applies this attribute as expected.
  • Reason this comment was not posted:
    Confidence changes required: 0% <= threshold 50% None
3. frontend/src/lib/components/meltComponents/Menu.svelte:63
  • Draft comment:
    Generic 'data-menu' attributes were removed from both the trigger button and the menu container. This prevents unintended selection across multiple menus—ensure that the new attribute (likely set via melt) is consistently applied.
  • Reason this comment was not posted:
    Confidence changes required: 0% <= threshold 50% None

Workflow ID: wflow_mxJSpXcWtB1Ef1KH

You can customize Ellipsis by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.

Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

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

Important

Looks good to me! 👍

Reviewed dddcb59 in 42 seconds. Click for details.
  • Reviewed 16 lines of code in 1 files
  • Skipped 0 files when reviewing.
  • Skipped posting 1 draft comments. View those below.
  • Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. frontend/src/lib/components/meltComponents/Menu.svelte:42
  • Draft comment:
    Switching to getElementById ensures only one element is returned. Ensure that the menu element has its id attribute set to $menuId, as this change assumes unique id assignment.
  • Reason this comment was not posted:
    Confidence changes required: 33% <= threshold 50% None

Workflow ID: wflow_dQ6N179uUsZcGkUJ

You can customize Ellipsis by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.

Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

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

Important

Looks good to me! 👍

Reviewed e1dbc53 in 1 minute and 5 seconds. Click for details.
  • Reviewed 160 lines of code in 4 files
  • Skipped 0 files when reviewing.
  • Skipped posting 6 draft comments. View those below.
  • Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. frontend/src/lib/components/apps/components/buttons/AppButton.svelte:41
  • Draft comment:
    New props (modalId, onSuccess, modalWrapperClass) added for modal config. Ensure their behavior and documentation align with intended modal usage.
  • Reason this comment was not posted:
    Confidence changes required: 33% <= threshold 50% None
2. frontend/src/lib/components/apps/components/buttons/AppButton.svelte:217
  • Draft comment:
    Invoking onSuccess callback immediately after updating listInputs—verify this doesn’t trigger duplicate side effects in multiple event flows.
  • Reason this comment was not posted:
    Comment did not seem useful. Confidence is useful = 0% <= threshold 50% The comment is asking the PR author to verify that invoking the onSuccess callback doesn't trigger duplicate side effects. This falls under asking the author to double-check things, which is against the rules. The comment does not provide a specific suggestion or point out a clear issue, so it should be removed.
3. frontend/src/lib/components/common/confirmationModal/ConfirmationModal.svelte:27
  • Draft comment:
    New modalId and wrapperClass props are added to the modal. Confirm these props are correctly propagated to avoid conflicts with multiple modals.
  • Reason this comment was not posted:
    Confidence changes required: 33% <= threshold 50% None
4. frontend/src/lib/components/apps/components/display/AppMenu.svelte:43
  • Draft comment:
    A unique modalId is generated with crypto.randomUUID. Ensure this approach remains consistent (e.g., with SSR/hydration) to avoid ID conflicts.
  • Reason this comment was not posted:
    Confidence changes required: 33% <= threshold 50% None
5. frontend/src/lib/components/meltComponents/Menu.svelte:18
  • Draft comment:
    The clickOutsideExcludeIds prop is introduced to exclude modal elements from outside click detection. Consider adding inline docs for future maintainers.
  • Reason this comment was not posted:
    Confidence changes required: 33% <= threshold 50% None
6. frontend/src/lib/components/meltComponents/Menu.svelte:95
  • Draft comment:
    Passing the 'close' function via the slot (changed from slot {item} to slot {item} {close}) allows nested components to close the menu. This is a good improvement.
  • Reason this comment was not posted:
    Confidence changes required: 33% <= threshold 50% None

Workflow ID: wflow_oqZdniWmdYSLOFLw

You can customize Ellipsis by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.

…ent-multiple-menu-opening

# Conflicts:
#	frontend/src/lib/components/common/confirmationModal/ConfirmationModal.svelte
#	frontend/src/lib/components/meltComponents/Menu.svelte
…ent-multiple-menu-opening

# Conflicts:
#	frontend/src/lib/components/apps/components/display/AppMenu.svelte
@Guilhem-lm
Copy link
Contributor Author

This PR fixes #5739

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.

1 participant