-
Notifications
You must be signed in to change notification settings - Fork 14.1k
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(dnd&column): make to fix the blank state issue when only one column select #19651
Conversation
Codecov Report
@@ Coverage Diff @@
## master #19651 +/- ##
==========================================
- Coverage 66.30% 66.30% -0.01%
==========================================
Files 1681 1681
Lines 64408 64406 -2
Branches 6593 6593
==========================================
- Hits 42704 42702 -2
Misses 20020 20020
Partials 1684 1684
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
col => | ||
!optionSelector.values | ||
.filter(isColumnMeta) | ||
.map((val: ColumnMeta) => val.column_name) | ||
.includes(col.column_name), |
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.
Did you confirm that we don't have other side effects by removing this filter?
If we really don't need this filter, can you add the reason into PR description so others can get an idea?
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.
It was supposed to filter out the current value from options when editing metric/column/filter. I suppose we don't really need it so lgtm 👍
/testenv up |
@kgabryje Ephemeral environment spinning up at http://54.187.169.217:8080. Credentials are |
() => Object.values(options), | ||
[optionSelector.values, options], |
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.
Isn't optionSelector
redundant in the dependency array after this?
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.
Approving. If we can apply @villebro's suggestion before 1.5 cut then let's do it, otherwise we can merge and open a followup PR
Ephemeral environment shutdown and build artifacts deleted. |
…mn select (apache#19651) (cherry picked from commit c320c29)
🏷️ preset:2022.15 |
SUMMARY
Single Temporal Column Should Appear in a dropdown instead of Blank State
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
BEFORE:
AFTER:
TESTING INSTRUCTIONS
How to reproduce bugs
ADDITIONAL INFORMATION