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

Don't propagate click events from popover + remove unnecessary overflow hidden #23443

Merged
merged 2 commits into from
Aug 6, 2024

Conversation

salazarm
Copy link
Contributor

@salazarm salazarm commented Aug 6, 2024

Summary & Motivation

  1. Don't propagate click events from the popover of ProductTour since by default blueprint's Popover seems to bubble them up to the element its wrapping (super weird!).
  2. Remove overflow: hidden from the AssetPageHeader because this wraps a button that uses outerglow on hover and the overflow hidden ends up hiding the outerglow making it look weird.

@salazarm salazarm requested review from bengotow and hellendag August 6, 2024 17:44
@salazarm salazarm marked this pull request as ready for review August 6, 2024 17:44
Copy link

github-actions bot commented Aug 6, 2024

Deploy preview for dagit-storybook ready!

✅ Preview
https://dagit-storybook-hzq4nkkfk-elementl.vercel.app
https://salazarm-remove-overflow-hidden.components-storybook.dagster-docs.io

Built with commit c953085.
This pull request is being automatically deployed with vercel-action

Copy link

github-actions bot commented Aug 6, 2024

Deploy preview for dagit-core-storybook ready!

✅ Preview
https://dagit-core-storybook-buw0qa476-elementl.vercel.app
https://salazarm-remove-overflow-hidden.core-storybook.dagster-docs.io

Built with commit c953085.
This pull request is being automatically deployed with vercel-action

@hellendag
Copy link
Member

Don't propagate click events from the popover of ProductTour since by default blueprint's Popover seems to bubble them up to the element its wrapping (super weird!).

What happens in this case when the event bubbles up? Is there some unexpected behavior?

Remove overflow: hidden from the AssetPageHeader because this wraps a button that uses outerglow on hover and the overflow hidden ends up hiding the outerglow making it look weird.

Can you share a screenshot of what this looks like? I'm not sure I'm visualizing it correctly.

@salazarm
Copy link
Contributor Author

salazarm commented Aug 6, 2024

@hellendag

What happens in this case when the event bubbles up? Is there some unexpected behavior?

So the product tour has a "Dismiss button" and when you click it, the click event propagates up to the button that the popover is wrapping which ends up causing the popover on the button to fire. It might be an issue due to have two popovers...

Can you share a screenshot of what this looks like? I'm not sure I'm visualizing it correctly.

https://www.notion.so/dagster/Dogfooding-Catalog-Views-8bf1432578f5465e84e35953afa36f38?pvs=4#5974a15e67bb4edb884c0ee69cb13e2c

Screenshot 2024-08-06 at 2 03 06 PM

flex={{alignItems: 'center', gap: 4}}
style={{maxWidth: '600px', overflow: 'hidden', marginBottom: 4}}
>
<Box flex={{alignItems: 'center', gap: 4}} style={{maxWidth: '600px', marginBottom: 4}}>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Interesting - verified this wasn't from Dish's latest PR about the truncating breadcrumbs, so it seems fine to remove to me 👍

@salazarm salazarm merged commit f3ce9a8 into master Aug 6, 2024
3 checks passed
@salazarm salazarm deleted the salazarm/remove-overflow-hidden branch August 6, 2024 19:12
jmsanders pushed a commit that referenced this pull request Aug 7, 2024
…ow hidden (#23443)

## Summary & Motivation

1. Don't propagate click events from the popover of ProductTour since by
default blueprint's `Popover` seems to bubble them up to the element its
wrapping (super weird!).
2. Remove overflow: hidden from the AssetPageHeader because this wraps a
button that uses outerglow on hover and the overflow hidden ends up
hiding the outerglow making it look weird.

(cherry picked from commit f3ce9a8)
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.

3 participants