Sortable component #4199
Sortable component #4199
Conversation
…o wrapped component)
… issues on Parameters component
There was a problem hiding this comment.
Thanks for doing this @kravets-levko, I should've taken a step forward and abstracted this when working with Parameters 👍
| // - update container element: add classes and take a ref | ||
| containerProps.className = cx( | ||
| 'sortable-container', | ||
| { 'sortable-container-dragging': isDragging }, |
There was a problem hiding this comment.
Why not a data-dragging modifier instead? I thought it was becoming a standard 🙂
There was a problem hiding this comment.
I think because we don't use data- attributes anywhere except of data-test. Also, IMHO class-based selectors are easier to read in CSS files
There was a problem hiding this comment.
I use it all over the place 😬
Imo, as long as data-prop1={condition || null}, the selector is as coherent in the CSS
[data-prop1] {
/* rule */
}There was a problem hiding this comment.
Well, I think we need some convention for this 😁 I personally don't like (and avoid to use) attribute-based selectors for few reasons:
- performance (all browsers have some optimizations for id/class-based selectors, but don't optimize tag/attribute based selectors)
- attributes selectors have a least specificity, so they're very easy to override (especially accidentally) by classes, but almost no way to override other rules by attribute-based selector.
- (as a part of previous one)
classattribute is a list and may contain multiple values (the next class overrides rules of previous. Data attributes selectors usually are used by 1) presense 2) exact value - so classes are more flexible.
I feel comfortable to use attributes for things that are standalone and "boolean" in their nature (like disabled property). In this case, we actually have two flags: draggable which defines some styles for draggable container (another thing that feels wrong to me - draggable flag should be set for element which is draggable itself), and dragged which depends on and overrides some styles of draggable - so actually these two attributes were used as classes - when second one extends/overrides previous one.
Actually it's a quite good topic for discussion 🤔
There was a problem hiding this comment.
I think these are all good points.
I personally have not encountered these potential downsides, but I appreciate the thoughtfulness and am ok with the approach.
There was a problem hiding this comment.
🤔 personally I prefer the data attributes, but I haven't taken the opportunity to answer your arguments yet.
We should find a convention, but this shouldn't block this PR.
| // - update container element: add classes and take a ref | ||
| containerProps.className = cx( | ||
| 'sortable-container', | ||
| { 'sortable-container-dragging': isDragging }, |
| // - update container element: add classes and take a ref | ||
| containerProps.className = cx( | ||
| 'sortable-container', | ||
| { 'sortable-container-dragging': isDragging }, |
There was a problem hiding this comment.
🤔 personally I prefer the data attributes, but I haven't taken the opportunity to answer your arguments yet.
We should find a convention, but this shouldn't block this PR.
What type of PR is this? (check all applicable)
Description
Currently the only place where we used drag'n'drop sorting was
Parameterscomponent: we usedreact-sortable-hocfor DnD and also added some nice animation effects to it. Now we need the same for Chart (#4139) and Table (#4175) editors. This PR decouples that custom features fromParameterscomponents and adds a basic implementation for "sortable" components which should help with repeatable operations (like maintaining "isDragged" state and corresponding classes, defining a default container, or ability to disable drag'n'drop).Also, this PR:
react-sortable-hoc(from1.9.1to1.10.1) - previous version had a bug (sortableContainerHOC was passing wrong properties to wrapped component) which is fixed in new version.Mobile & Desktop Screenshots/Recordings (if there are UI changes)
Before:
After:
