-
Notifications
You must be signed in to change notification settings - Fork 14k
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: Filters alert width #24801
fix: Filters alert width #24801
Conversation
Codecov Report
@@ Coverage Diff @@
## master #24801 +/- ##
=======================================
Coverage 68.95% 68.96%
=======================================
Files 1902 1902
Lines 73998 73999 +1
Branches 8195 8195
=======================================
+ Hits 51029 51030 +1
Misses 20850 20850
Partials 2119 2119
Flags with carried forward coverage won't be shown. Click here to find out more.
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
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.
Looks good to me other than 1000px adjustment change (will be reviewed @kasiazjc
I think on the smaller screens, modals that are that big of a size can be overwhelming (that's why I would prefer to avoid them) and I do not think that we add that much value with the increased size (especially because with 880x the fields are already bigger than our default + 240px seems like enough). I think we use width of 880px we use in the dashboard properties etc and the 1000px+ is used for alerts&reports as there is a lot of content and different inputs etc. Can be convinced that 1000px is better though if you give me reasons @michael-s-molina :D |
I was worried it wouldn't work for Airbnb but I checked with @justinpark and 880px is fine for most cases. I changed the width back to 880px. |
Thank you @michael-s-molina ❤️ |
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 ✨
(cherry picked from commit 4b1f1d4)
SUMMARY
Fixes the filters alert width and also changes the modal width.
Following @kasiazjc's comment in #24559:
I adjusted the modal to have a fixed base width instead of percent based one. I set it to 1000px instead of 880px as a middle ground.
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
TESTING INSTRUCTIONS
Make sure the the width of the filters' alert takes the available space.
ADDITIONAL INFORMATION