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.

Sign up for GitHub

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

Jump to bottom

Components: Draggable, add story #18070

Merged
merged 3 commits into from
Oct 29, 2019
Merged

Components: Draggable, add story #18070

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
2b3b17f
Components: Add Story for Draggable
Oct 22, 2019
5b1b274
Merge branch 'master' into add/draggable-story
Oct 28, 2019
a0b275d
Fix useState hook for Draggable story example
Oct 28, 2019
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
69 changes: 69 additions & 0 deletions packages/components/src/draggable/stories/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,69 @@
/**
* WordPress dependencies
*/
import { useState } from '@wordpress/element';

/**
* Internal dependencies
*/
import Draggable from '../';
import Dashicon from '../../dashicon';

export default { title: 'Draggable', component: Draggable };

const Box = ( props ) => {
Copy link
Member

Choose a reason for hiding this comment

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

Yet another reason to introduce a Box component which would work cross-platform :)

return (
<div
{ ...props }
style={ {
alignItems: 'center',
display: 'flex',
justifyContent: 'center',
width: 100,
height: 100,
background: '#ddd',
} }
/>
);
};

const DraggalbeExample = () => {
const [ isDragging, setDragging ] = useState( false );
Copy link
Member

Choose a reason for hiding this comment

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

According to documentation, the function exported as a story isn't a React component. It means that we shouldn't use hooks in this context as they might stop to work in the near future if they change the way all that works.

The easiest way to get rid of this warning is to move the part which manages state out of this function. See a similar usage:

gutenberg/packages/components/src/checkbox-control/stories/index.js

Lines 18 to 28 in a6e1252

const CheckboxControlWithState = ( { checked, ...props } ) => {
const [ isChecked, setChecked ] = useState( checked );
return (
<CheckboxControl
{ ...props }
checked={ isChecked }
onChange={ setChecked }
/>
);
};

In this case, it might be more work to do it :(


// Allow for the use of ID in the example
/* eslint-disable no-restricted-syntax */
return (
<div>
<p>Is Dragging? { isDragging ? 'Yes' : 'No' }</p>
<div id="draggable-example-box" style={ { display: 'inline-flex' } }>
Copy link
Member

@gziolo gziolo Oct 29, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

I'm a bit concerned about the implementation of the Drabbable component. Why do you need to wrap it with a div which has the id which is referenced with elementId prop?

It's completely unrelated to this PR, but I'd be in favor of opening a follow-up issue which makes it easier to use. In effect, we should seek ways to make the need to use id an internal implementation. This would also ensure that there is no id duplication introduced when 2 instances of the same component are used. Not mentioning, that disabling the ESLint rule is bad experience for the implementer.

Taking the example from README:

import { Dashicon, Draggable, Panel, PanelBody } from '@wordpress/components';

const MyDraggable = ( { onDragStart, onDragEnd } ) => (
	<div id="draggable-panel">
		<Panel header="Draggable panel" >
			<PanelBody>
				<Draggable
					elementId="draggable-panel"
					transferData={ { } }
					onDragStart={ onDragStart }
					onDragEnd={ onDragEnd }
				>
				{
					( { onDraggableStart, onDraggableEnd } ) => (
						<Dashicon
							icon="move"
							onDragStart={ onDraggableStart }
							onDragEnd={ onDraggableEnd }
							draggable
						/>
					)
				}
				</Draggable>
			</PanelBody>
		</Panel>
	</div>
);

I would love us to explore whether it could be refactored to:

import { Dashicon, Draggable, Panel, PanelBody } from '@wordpress/components';

const MyDraggable = ( { onDragStart, onDragEnd } ) => (
	<Draggable
		as={ Panel }
		header="Draggable panel"
		transferData={ { } }
		onDragStart={ onDragStart }
		onDragEnd={ onDragEnd }
	>
		{ ( { onDraggableStart, onDraggableEnd } ) => (
			<PanelBody>
				<Dashicon
					icon="move"
					onDragStart={ onDraggableStart }
					onDragEnd={ onDraggableEnd }
					draggable
				/>
			</PanelBody>
		) }
	</Draggable>
);

/cc @nosolosw

Copy link
Member

Choose a reason for hiding this comment

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

There is also a related issue with draggable elements as explained in:
#18158

Copy link
Author

Choose a reason for hiding this comment

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

Why do you need to wrap it with a div which has the id which is referenced with elementId prop?

@gziolo I noticed that too. I found it a bit unintuitive/restrictive in my first couple attempts of getting Draggable to work.

The above as technique is cool! And if as is not defined, maybe it can fallback to a regular <div>, wrapping the children that has been passed in.

Copy link
Member

Choose a reason for hiding this comment

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

This is what VisuallyHidden does:

gutenberg/packages/components/src/visually-hidden/index.js

Line 12 in d56979d

as = 'div',

The above is a cool collaboration between @mkaz and @diegohaz 💯

<Draggable elementId="draggable-example-box">
{ ( { onDraggableStart, onDraggableEnd } ) => {
const handleOnDragStart = ( event ) => {
setDragging( true );
onDraggableStart( event );
};
const handleOnDragEnd = ( event ) => {
setDragging( false );
onDraggableEnd( event );
};

return (
<Box
onDragStart={ handleOnDragStart }
onDragEnd={ handleOnDragEnd }
draggable
>
<Dashicon icon="move" />
</Box>
);
} }
</Draggable>
</div>
</div>
);
/* eslint-enable no-restricted-syntax */
};

export const _default = () => {
return <DraggalbeExample />;
};