-
Notifications
You must be signed in to change notification settings - Fork 497
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
fix(@clayui/shared): LPD-48347 Close overlay on left click only #5933
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
Hey @pat270 I think this would not be ideal to do because it is not common user behavior, especially if we are doing this for debugging. Would that be the intention? The ideal here would be to just add a breakpoint to do the debugging on the js side to avoid the dropdown closing if that were the case. I don't want to add that behavior if that's the case. |
@matuzalemsteles Not everyone has access to the source code. It's pretty painful having to search through the elements to locate the correct dropdown. |
@pat270 The debugger can be done by the browser itself, there is no need to go into the code to modify it. We can create a guide for this if necessary, but I don't think it's worth having to make this change just to save the developer's experience and harm the user's experience. This is a very common behavior for the user, so I'm not sure about it. Just to understand what we want to do here, do you want the overlay to be closed only by left-clicking anywhere on the screen, but should it remain open when right-clicking? |
I mean when you right click on a page, the browser's context menu to appears and a selection shouldn't cause changes on the page. Look at github's dropdown menus for example, it functions correctly imo. |
Hmm I think I get your point but now testing this behavior in Clay I was able to do the same for example in storybook https://deploy-preview-5933--next-storybook-clayui.netlify.app/?path=/story/design-system-components-dropdown--cascading-menu. Now with the changes in this PR I can't close the menu by left-clicking outside the dropdown. Am I missing something here? |
@matuzalemsteles This should work now. |
@pat270 I can't see this case being reproduced in the master. Right-clicking still allows me to open the browser menu and not close the dropdown https://storybook.clayui.com/. Is there any step I can reproduce in the master? |
@matuzalemsteles You have to right click outside the dropdown menu |
@pat270 Ok I think I understand what you want here but I think this will break the behavior we added to fix another issue #5740 in case you want to remove the behavior of closing the menu when changing tabs or clicking outside the window right? I think to go with this we need to test that this won't cause a regression in this use case, can you check that? |
@matuzalemsteles It doesn't break it. |
@pat270 we need to check the use case in DXP so we will have to monitor this there when the release comes out with this change, if I remember correctly we had some problems in not being able to properly reproduce this bug in the storybook because in DXP there was interference from other events. |
Please make sure to test this in DXP before releasing by following this process: https://github.com/liferay/clay/blob/master/CONTRIBUTING.md#testing-local-changes-in-liferay-portal |
https://liferay.atlassian.net/browse/LPD-48347