-
Notifications
You must be signed in to change notification settings - Fork 26
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
[Bug 1885504] Improve automatic click events for nested elements #1895
Conversation
- Glean click events: Remove pointer-events:none for nested elements
- Glean click events: Remove pointer-events:none for nested elements
Tested this change in following apps:
With the approach proposed in this PR, I was able to remove As per my understanding, the theme toggle button in Debug Ping View is a special case. As per the theme toggler code: <button style={{ all: 'unset', cursor: 'pointer' }} data-glean-label='Theme' onClick={toggleTheme}>
{isDarkMode ? <SunIcon /> : <MoonIcon />}
</button> This means, when the current theme is "dark", the <button style={{ all: 'unset', cursor: 'pointer' }} data-glean-label='Theme' onClick={toggleTheme}>
<SunIcon />
</button> When user clicks the button to toggle the theme from "dark" to "light", the target of the click event becomes <button style={{ all: 'unset', cursor: 'pointer' }} data-glean-label='Theme' onClick={toggleTheme}>
<MoonIcon />
</button> As |
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.
I left a few comments here. Nothing too major, just a few callouts and different ways to do stuff.
- Removed log statements - More concise constructClickEventContextForElement
Hey @rosahbruno Thanks a lot for review. I submitted new commits to address your comments 👍🏾 Btw, do you have any opinion on this 👇🏾 ?
|
I think this is fine. The theme toggle is a bit of an odd set up since it toggles between two separate components. I think having to use a small workaround for something like that still, as long as these changes fix other things, then it should be ok |
Great. As I mentioned above, this change enables us to remove |
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, nice job
I tested this on the Glean samples/browser/web app and it is working as expected. I think we should add this by default to this sample project. I will make a bug and a PR for this since I already have it done.
Thanks @rosahbruno 👍🏾 |
Fixes https://bugzilla.mozilla.org/show_bug.cgi?id=1885504
Pull Request checklist
glean/
folder, run:npm run test
Runs all testsnpm run lint
Runs all lintersCHANGELOG.md
or an explanation of why it does not need onemozilla/glean
repository