-
Notifications
You must be signed in to change notification settings - Fork 13.8k
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(explore): Hide non-droppable metric and column list #27717
chore(explore): Hide non-droppable metric and column list #27717
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #27717 +/- ##
==========================================
+ Coverage 67.47% 67.50% +0.02%
==========================================
Files 1911 1913 +2
Lines 75020 75142 +122
Branches 8355 8403 +48
==========================================
+ Hits 50621 50722 +101
- Misses 22349 22358 +9
- Partials 2050 2062 +12
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
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.
Thank you for the PR @justinpark.
const allowedColumns = useMemo(() => { | ||
const validators = Object.values(dropzones); | ||
if (!isArray(_columns)) return []; | ||
return allowlistOnly |
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.
@justinpark If we have the concept of validators where every item is checked to see if it's allowed, similar to a security check, wouldn't be the responsibility of the validator to check for disallow_adhoc_metrics
? In other words, could we always validate the items with the default implementation being that all items are allowed? This would give us the flexibility to show/hide items depending on any type of condition.
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.
In other words, could we always validate the items with the default implementation being that all items are allowed?
@michael-s-molina As mentioned earlier, in the adhoc query, all columns and metrics are accepted. Therefore, applying additional filtering in cases where non-disallowed-adhoc
case can be considered unnecessary.
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.
We synced offline and will ensure that these filters are not affected by the 'disallow-adhoc' condition.
92ea7fe
to
7fb999a
Compare
/testenv up |
@justinpark I think it would be important to show some kind of message/feedback to users when we have hidden columns or metrics because they can't be used anywhere. Maybe @kasiazjc can help us here. |
@michael-s-molina If you are interested in considering feedback, this proposal suggests adopting a Gmail-style approach. cc: @kasiazjc |
This looks nice @justinpark. I would only keep the background white to not mix it with the draggable items and because this looks like a secondary information that does not need to be highlighted frequently. Once users view this for the first time, they will get used to it. We won't have the Undo link right? I'm assuming that you can't undo the applicability of columns because they depend on what's already configured in the controls. |
Sounds good. I'll update accordingly.
I agree with your opinion. Let me remove "undo" now. |
/testenv up |
@michael-s-molina Ephemeral environment spinning up at http://35.94.26.119:8080. Credentials are |
@justinpark If you resize the left panel, it's cutting the text. |
@michael-s-molina I fixed by the text ellipsis. |
Ephemeral environment shutdown and build artifacts deleted. |
(cherry picked from commit eda304b)
(cherry picked from commit c06f068)
SUMMARY
There can be certain items that cannot be accepted in all options. (For instance, metrics cannot be accepted in the columns selection. In cases where the controls solely consist of the columns selection, none of the metrics will be accepted in the overall dropzone, as shown in the screenshot below.)
To address this, the commit takes a step forward by checking the eligibility of each filter and hiding the item when there are no eligible options available.
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
Before:
before--hide-disallow-list.mov
After:
after--hide-disallow-list.mov
TESTING INSTRUCTIONS
ADDITIONAL INFORMATION