-
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(explore): reordering columns with dnd sometimes glitching #16322
Conversation
Codecov Report
@@ Coverage Diff @@
## master #16322 +/- ##
==========================================
- Coverage 76.65% 76.65% -0.01%
==========================================
Files 999 999
Lines 53397 53391 -6
Branches 6819 6818 -1
==========================================
- Hits 40933 40926 -7
- Misses 12227 12229 +2
+ Partials 237 236 -1
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
/testenv up FEATURE_ENABLE_EXPLORE_DRAG_AND_DROP=true FEATURE_ENABLE_DND_WITH_CLICK_UX=true |
@kgabryje Ephemeral environment spinning up at http://34.214.208.78:8080. Credentials are |
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!
const item: OptionItemInterface = useMemo( | ||
() => ({ | ||
dragIndex: index, | ||
type, | ||
}), | ||
[index, type], | ||
); | ||
const [{ isDragging }, drag] = useDrag({ | ||
item, | ||
item: { | ||
type, | ||
dragIndex: index, | ||
}, |
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 appears we're a few versions behind react-dnd
(now on 14; we're on 11). Apparently the deps can now be given as a param after spec:
https://react-dnd.github.io/react-dnd/docs/api/use-drag
Might be a good TODO for when we bump this the next time.
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.
Good to know!
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
reordering.in.column.mov
reordering.in.filters.mov
Ephemeral environment shutdown and build artifacts deleted. |
🏷️ 2021.31 |
…#16322) * fix(explore): reordering columns with dnd sometimes glitching * Fix metrics and filters popover being stale after reordering
…#16322) * fix(explore): reordering columns with dnd sometimes glitching * Fix metrics and filters popover being stale after reordering
SUMMARY
When user added 3 or more columns to a control that accepts only columns (such as Group by) and started reordering using drag and drop, the feature was starting to glitch after the first sorting.
The first dragging and dropping a column to reorder worked correctly. Due to memoization, when user wanted to reorder the second time, the drop index from the first reordering was being used as a start index of the next reordering. That caused a glitch, which resulted in columns "jumping" and changing their positions incorrectly.
Another issue related to reordering with dnd happened in filters and metrics controls. After reordering, when user clicked a metric label, the popover's content was stale - it showed the column name and aggregate of a metric that was at current index before reordering. The source of the problem seems to be antd popover's internal memoization, which we can switch off by setting a prop
destroyTooltipOnHide
. Before, that prop was set only for popover used to create a new metric or filter, so that user doesn't see old selections after creating a metric/filter and clicking again. Now we set that prop also for popovers used for editing metrics and filters.BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
Before: see linked issue
After:
https://user-images.githubusercontent.com/15073128/129889010-9e5c5a1e-28f7-41e2-a3b0-46bf6d446b86.mov
TESTING INSTRUCTIONS
ADDITIONAL INFORMATION
CC @junlincc @jinghua-qa