-
Notifications
You must be signed in to change notification settings - Fork 34
[Bug 1885504] Improve automatic click events for nested elements #1895
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
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 |
rosahbruno
left a comment
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 |
rosahbruno
left a comment
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 testRuns all testsnpm run lintRuns all lintersCHANGELOG.mdor an explanation of why it does not need onemozilla/gleanrepository