Skip to content
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

Sortable component #4199

Merged
merged 3 commits into from
Sep 30, 2019
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
51 changes: 17 additions & 34 deletions client/app/components/Parameters.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ import React from 'react';
import PropTypes from 'prop-types';
import { size, filter, forEach, extend } from 'lodash';
import { react2angular } from 'react2angular';
import { sortableContainer, sortableElement, sortableHandle } from 'react-sortable-hoc';
import { SortableContainer, SortableElement, DragHandle } from '@/components/sortable';
import { $location } from '@/services/ng';
import { Parameter } from '@/services/query';
import ParameterApplyButton from '@/components/ParameterApplyButton';
Expand All @@ -12,18 +12,6 @@ import { toHuman } from '@/filters';

import './Parameters.less';

const DragHandle = sortableHandle(({ parameterName }) => (
<div className="drag-handle" data-test={`DragHandle-${parameterName}`} />
));

const SortableItem = sortableElement(({ className, parameterName, disabled, children }) => (
<div className={className} data-editable={!disabled || null}>
{!disabled && <DragHandle parameterName={parameterName} />}
{children}
</div>
));
const SortableContainer = sortableContainer(({ children }) => children);

function updateUrl(parameters) {
const params = extend({}, $location.search());
parameters.forEach((param) => {
Expand All @@ -50,12 +38,12 @@ export class Parameters extends React.Component {
onValuesChange: () => {},
onPendingValuesChange: () => {},
onParametersEdit: () => {},
}
};

constructor(props) {
super(props);
const { parameters } = props;
this.state = { parameters, dragging: false };
this.state = { parameters };
if (!props.disableUrlUpdate) {
updateUrl(parameters);
}
Expand Down Expand Up @@ -101,11 +89,6 @@ export class Parameters extends React.Component {
return { parameters };
});
}
this.setState({ dragging: false });
};

onBeforeSortStart = () => {
this.setState({ dragging: true });
};

applyChanges = () => {
Expand Down Expand Up @@ -170,32 +153,32 @@ export class Parameters extends React.Component {
}

render() {
const { parameters, dragging } = this.state;
const { parameters } = this.state;
const { editable } = this.props;
const dirtyParamCount = size(filter(parameters, 'hasPendingValue'));
return (
<SortableContainer
disabled={!editable}
axis="xy"
useDragHandle
lockToContainerEdges
helperClass="parameter-dragged"
updateBeforeSortStart={this.onBeforeSortStart}
onSortEnd={this.moveParameter}
containerProps={{
className: 'parameter-container',
onKeyDown: dirtyParamCount ? this.handleKeyDown : null,
}}
>
<div
className="parameter-container"
onKeyDown={dirtyParamCount ? this.handleKeyDown : null}
data-draggable={editable || null}
data-dragging={dragging || null}
>
{parameters.map((param, index) => (
<SortableItem className="parameter-block" key={param.name} index={index} parameterName={param.name} disabled={!editable}>
{parameters.map((param, index) => (
<SortableElement key={param.name} index={index}>
<div className="parameter-block" data-editable={editable || null}>
{editable && <DragHandle data-test={`DragHandle-${param.name}`} />}
{this.renderParameter(param, index)}
</SortableItem>
))}

<ParameterApplyButton onClick={this.applyChanges} paramCount={dirtyParamCount} />
</div>
</div>
</SortableElement>
))}
<ParameterApplyButton onClick={this.applyChanges} paramCount={dirtyParamCount} />
</SortableContainer>
);
}
Expand Down
37 changes: 11 additions & 26 deletions client/app/components/Parameters.less
Original file line number Diff line number Diff line change
@@ -1,32 +1,24 @@
@import '../assets/less/ant';

.drag-handle {
background: linear-gradient(90deg, transparent 0px, white 1px, white 2px)
center,
linear-gradient(transparent 0px, white 1px, white 2px) center, #111111;
background-size: 2px 2px;
display: inline-block;
width: 6px;
height: 36px;
vertical-align: bottom;
margin-right: 5px;
cursor: move;
}

.parameter-block {
display: inline-block;
background: white;
padding: 0 12px 6px 0;
vertical-align: top;
kravets-levko marked this conversation as resolved.
Show resolved Hide resolved

.parameter-container[data-draggable] & {
.drag-handle {
padding: 0 5px;
margin-left: -5px;
height: 36px;
}

.parameter-container.sortable-container & {
margin: 4px 0 0 4px;
padding: 3px 6px 6px;
}

&.parameter-dragged {
box-shadow: 0 4px 9px -3px rgba(102, 136, 153, 0.15);
width: auto !important;
}
}

Expand All @@ -53,20 +45,13 @@
.parameter-container {
position: relative;

&[data-draggable] {
&.sortable-container {
padding: 0 4px 4px 0;
transition: background-color 200ms ease-out;
transition-delay: 300ms; // short pause before returning to original bgcolor
}

&[data-dragging] {
transition-delay: 0s;
background-color: #f6f8f9;
}

.parameter-apply-button {
display: none; // default for mobile

// "floating" on desktop
@media (min-width: 768px) {
position: absolute;
Expand All @@ -83,7 +68,7 @@
display: block;
pointer-events: none; // so tooltip doesn't remain after button hides
}

&[data-show="true"] {
opacity: 1;
display: block;
Expand Down Expand Up @@ -118,7 +103,7 @@
line-height: 15px;
background: #f77b74;
border-radius: 7px;
box-shadow: 0px 0px 0 1px white, -1px 1px 0 1px #5d6f7d85;
box-shadow: 0 0 0 1px white, -1px 1px 0 1px #5d6f7d85;
}
}
}
80 changes: 80 additions & 0 deletions client/app/components/sortable/index.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,80 @@
import { isFunction, wrap } from 'lodash';
import React, { useRef, useState } from 'react';
import PropTypes from 'prop-types';
import cx from 'classnames';
import { sortableContainer, sortableElement, sortableHandle } from 'react-sortable-hoc';

import './style.less';

export const DragHandle = sortableHandle(({ className, ...restProps }) => (
<div className={cx('drag-handle', className)} {...restProps} />
));

export const SortableContainerWrapper = sortableContainer(({ children }) => children);

export const SortableElement = sortableElement(({ children }) => children);

export function SortableContainer({ disabled, containerProps, children, ...wrapperProps }) {
const containerRef = useRef();
const [isDragging, setIsDragging] = useState(false);

wrapperProps = { ...wrapperProps };
containerProps = { ...containerProps };

if (disabled) {
// Disabled state:
// - forbid drag'n'drop (and therefore no need to hook events
// - don't override anything on container element
wrapperProps.shouldCancelStart = () => true;
} else {
// Enabled state:

// - use container element as a default helper element
wrapperProps.helperContainer = wrap(wrapperProps.helperContainer, helperContainer => (
isFunction(helperContainer) ?
helperContainer(containerRef.current) :
containerRef.current
));

// - hook drag start/end events
wrapperProps.updateBeforeSortStart = wrap(wrapperProps.updateBeforeSortStart, (updateBeforeSortStart, ...args) => {
setIsDragging(true);
if (isFunction(updateBeforeSortStart)) {
updateBeforeSortStart(...args);
}
});
wrapperProps.onSortEnd = wrap(wrapperProps.onSortEnd, (onSortEnd, ...args) => {
setIsDragging(false);
if (isFunction(onSortEnd)) {
onSortEnd(...args);
}
});

// - update container element: add classes and take a ref
containerProps.className = cx(
'sortable-container',
{ 'sortable-container-dragging': isDragging },
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not a data-dragging modifier instead? I thought it was becoming a standard 🙂

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 */
}

Copy link
Collaborator Author

@kravets-levko kravets-levko Sep 30, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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) class attribute 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 🤔

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤔 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.

containerProps.className,
);
containerProps.ref = containerRef;
}

// order of props matters - we override some of them
return (
<SortableContainerWrapper {...wrapperProps}>
kravets-levko marked this conversation as resolved.
Show resolved Hide resolved
<div {...containerProps}>{children}</div>
</SortableContainerWrapper>
);
}

SortableContainer.propTypes = {
disabled: PropTypes.bool,
containerProps: PropTypes.object, // eslint-disable-line react/forbid-prop-types
children: PropTypes.node,
};

SortableContainer.defaultProps = {
disabled: false,
containerProps: {},
children: null,
};
30 changes: 30 additions & 0 deletions client/app/components/sortable/style.less
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
.drag-handle {
vertical-align: bottom;
cursor: move;

display: inline-flex;
align-items: stretch;
justify-content: center;

&:before {
content: '';
display: block;
width: 6px;

background:
linear-gradient(90deg, transparent 0px, white 1px, white 2px) center,
linear-gradient(transparent 0px, white 1px, white 2px) center,
#111111;
background-size: 2px 2px;
}
}

.sortable-container {
transition: background-color 200ms ease-out;
transition-delay: 300ms; // short pause before returning to original bgcolor

&.sortable-container-dragging {
transition-delay: 0s;
background-color: #f6f8f9;
}
}
12 changes: 6 additions & 6 deletions package-lock.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -82,7 +82,7 @@
"react-dom": "^16.8.3",
"react-grid-layout": "git+https://github.com/getredash/react-grid-layout.git",
"react-pivottable": "^0.9.0",
"react-sortable-hoc": "^1.9.1",
"react-sortable-hoc": "^1.10.1",
"react2angular": "^3.2.1",
"tinycolor2": "^1.4.1",
"ui-select": "^0.19.8"
Expand Down