-
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
chore(native filters): Expandable filter config modal #24559
chore(native filters): Expandable filter config modal #24559
Conversation
@justinpark in addition to the scroll bars, is there merit in making the default modal size wider? |
Sure. I can adjust the default width from 50% to 70% instead of making the expanded mode as default because 70% width modal can be suitable in most case. |
Codecov Report
@@ Coverage Diff @@
## master #24559 +/- ##
=======================================
Coverage 69.03% 69.03%
=======================================
Files 1908 1908
Lines 74197 74221 +24
Branches 8186 8188 +2
=======================================
+ Hits 51219 51238 +19
- Misses 20857 20858 +1
- Partials 2121 2125 +4
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 |
width="70%" | ||
{...(expanded && { width: '100%', height: '100%' })} |
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.
Given that StyleModalWrapper
is already receiving the expanded
property, could you deal with width
and height
there and remove these two lines?
@michael-s-molina could you review the update? |
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
Thanks for the change @justinpark! I am worried that because the icons are so similar and close to each other, people might confuse them. I think in other places we use this icon (below) and using this one would probably help. Would it be ok to swap? I think in general we have to bump the width of the modal in the default mode to ~880px (and resize just after it not longer fits in the screen), so that everything is more visible. Do you think you could include it in the PR or should it be included in a new one? |
3773b89
to
6516328
Compare
Co-authored-by: Justin Park <justinpark@apache.org> (cherry picked from commit 05e724f)
Co-authored-by: Justin Park <justinpark@apache.org>
SUMMARY
The native filters modal width is unnecessarily narrow which results in a suboptimal UX—excessive scrolling—especially when either the filter names and/or datasets/column names are long.
This commit adds an option to make the 'Add and edit filters' modal wider.
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
After:
after-expandable-filter-modal.mov
Before:
before--expandable-filter-modal.mov
TESTING INSTRUCTIONS
Go to dashboard and choose a dataset contains a long name column name
ADDITIONAL INFORMATION