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

Menu: Fix that Buttons not worked as a activator content #3065

Merged
merged 2 commits into from
Oct 17, 2021

Conversation

mckaragoz
Copy link
Member

@mckaragoz mckaragoz commented Oct 16, 2021

Description

fixes #1583, fixes #3063.

Before this, when we click a button which as a activation, the popover didn't open, so menu didn't work.

With an investigate, i think the problem is, menu's activator content has onmouseup, but button's doesn't have, they instead of have onclick. So we may do two optional approach (both works):

  1. Change all button's onclick events to the onmouseup events.
  2. Change menu's onmouseup event to the onclick event.
    (We cant simply add another onmouseup event because this time when we click a button, onclick and onmouseup both fires twice)

I think the first approach is more problematic because i am sure it's a breaking change.

So i go ahead of the second option. When we change onmouseup to onclick, right click event didn't work (probably thats why we used onmouseup in menu), so i added the oncontextmenu events to succeded a working right click.

With this change, i expected that all contents, all variants and all click types will work with menu properly after this PR.

Note: Added docs that shows that working button as activator content.

How Has This Been Tested?

Types of changes

  • Bug fix (non-breaking change which fixes an issue)

Checklist:

  • The PR is submitted to the correct branch (dev).
  • My code follows the code style of this project.
  • I've added relevant tests.

@mckaragoz
Copy link
Member Author

mckaragoz commented Oct 16, 2021

@henon could you check this? I think with different aspects and it works with both BSS and WASM. Do you think there is another unexpected situation?

@Garderoben you commented on #1583 about a solution. But i think popover PR can't solve the #1583 alone, we also need this PR, am i right? (it's not only the z-index property)

@Garderoben
Copy link
Member

@Garderoben you commented on #1583 about a solution. But i think popover PR can't solve the #1583 alone, we also need this PR, am i right? (it's not only the z-index property)

Nah my comment stated that if the Buttons where set to Disabled="true" the activator content worked.
Popover PR will not solve this correct.

@henon
Copy link
Collaborator

henon commented Oct 17, 2021

sure looks good. Is this sufficiently covered by the test suite?

@mckaragoz
Copy link
Member Author

I will look at it, actually we should prepare a new test that check all possible situations

@mckaragoz mckaragoz added the bug Something does not work as intended/expected label Oct 17, 2021
@mckaragoz
Copy link
Member Author

@henon test added. Now it completely check all variants (button, iconbutton and activator content-renderfragment) left and right click. Now we have a crucial menu test.

But i don't know about codacy, it says remove the redundant characters.

@henon henon merged commit 4163e82 into MudBlazor:dev Oct 17, 2021
@henon
Copy link
Collaborator

henon commented Oct 17, 2021

Thanks! Good job.

@henon henon added this to the 5.2.0 milestone Oct 17, 2021
@mckaragoz mckaragoz deleted the MenuActivatorButton branch October 17, 2021 18:10
@ScarletKuro
Copy link
Member

I'm late, but looks like this configuration doesn't work when the ActivationEvent is LeftClick: https://try.mudblazor.com/snippet/GaQnkCusULmyTWfg

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something does not work as intended/expected
Projects
None yet
4 participants