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

Add AnglePicker Component; Add useDragging hook #19637

Merged
Merged
Show file tree
Hide file tree
Changes from all 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
103 changes: 103 additions & 0 deletions packages/components/src/angle-picker/index.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,103 @@
/**
* WordPress dependencies
*/
import { useRef } from '@wordpress/element';
import { useInstanceId, __experimentalUseDragging as useDragging } from '@wordpress/compose';
import { __ } from '@wordpress/i18n';

/**
* Internal dependencies
*/
import BaseControl from '../base-control';

function getAngle( centerX, centerY, pointX, pointY ) {
const y = pointY - centerY;
const x = pointX - centerX;

const angleInRadians = Math.atan2( y, x );
const angleInDeg = Math.round( angleInRadians * ( 180 / Math.PI ) ) + 90;
if ( angleInDeg < 0 ) {
return 360 + angleInDeg;
}
return angleInDeg;
}

const AngleCircle = ( { value, onChange, ...props } ) => {
const angleCircleRef = useRef();
const angleCircleCenter = useRef();

const setAngleCircleCenter = () => {
const rect = angleCircleRef.current.getBoundingClientRect();
angleCircleCenter.current = {
x: rect.x + ( rect.width / 2 ),
y: rect.y + ( rect.height / 2 ),
};
};

const changeAngleToPosition = ( event ) => {
const { x: centerX, y: centerY } = angleCircleCenter.current;
onChange( getAngle( centerX, centerY, event.clientX, event.clientY ) );
};

const { startDrag, isDragging } = useDragging( {
onDragStart: ( event ) => {
setAngleCircleCenter();
changeAngleToPosition( event );
},
onDragMove: changeAngleToPosition,
onDragEnd: changeAngleToPosition,
} );
return (
/* eslint-disable jsx-a11y/no-static-element-interactions */
<div
Copy link
Member

Choose a reason for hiding this comment

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

You can't control it with the keyboard at all. It would be great to get some feedback from the accessibility experts on the best practices here, cc @tellthemachines and @enriquesanchez.

Copy link
Member Author

Choose a reason for hiding this comment

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

Hi @gziolo, I added aria-hidden to the circular picker. It is hidden to screen reader users. We have a text field that allows setting the angle with the keyboard, the field also support arrows (up/down) to increase/decrease the angle. So I think we are covered we provide a UI for keyboard users and on for mouse users.

Copy link
Member

Choose a reason for hiding this comment

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

I don't mean to block all the great work in this PR. We can revisit it later. I just wanted to make sure it's properly tested.

By the way, I don't see aria-hidden used in this PR. What do I miss?

Copy link
Member Author

Choose a reason for hiding this comment

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

Sorry I missed the part where I actually save the file. It is fixed now 👍

ref={ angleCircleRef }
onMouseDown={ startDrag }
className="components-angle-picker__angle-circle"
style={ isDragging ? { cursor: 'grabbing' } : undefined }
{ ...props }
>
<div
style={ value ? { transform: `rotate(${ value }deg)` } : undefined }
className="components-angle-picker__angle-circle-indicator-wrapper"
>
<span className="components-angle-picker__angle-circle-indicator" />
</div>
</div>
/* eslint-enable jsx-a11y/no-static-element-interactions */
);
};

export default function AnglePicker( { value, onChange, label = __( 'Angle' ) } ) {
const instanceId = useInstanceId( AnglePicker );
const inputId = `components-angle-picker__input-${ instanceId }`;
return (
<BaseControl
label={ label }
id={ inputId }
className="components-angle-picker"
>
<AngleCircle
value={ value }
onChange={ onChange }
aria-hidden="true"
/>
<input
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a way we could use a control here?

I am guessing you used this raw input because you needed AngleCircle inside the base control. Maybe there is room for extending/adding children to controls?

Copy link
Member

Choose a reason for hiding this comment

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

RangeControl is the closest one:

https://github.com/WordPress/gutenberg/blob/master/packages/components/src/range-control/index.js

It has before and after icons as props, but it might be less work to keep it as is.

Copy link
Member Author

@jorgefilipecosta jorgefilipecosta Jan 23, 2020

Choose a reason for hiding this comment

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

I think adding children in controls is not something very common and is an edge case. Also, some people may prefer the children after the input others before it seems to make our API more complex. So I guess for now we can keep using the raw input as here we are implementing a control itself. Later if this becomes common we can try to abstract things.

className="components-angle-picker__input-field"
type="number"
id={ inputId }
onChange={ ( event ) => {
const unprocessedValue = event.target.value;
const inputValue = unprocessedValue !== '' ?
parseInt( event.target.value, 10 ) :
0;
onChange( inputValue );
} }
value={ value }
min={ 0 }
max={ 360 }
step="1"
/>
</BaseControl>
);
}

23 changes: 23 additions & 0 deletions packages/components/src/angle-picker/stories/index.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@

/**
* WordPress dependencies
*/
import { useState } from '@wordpress/element';

/**
* Internal dependencies
*/
import AnglePicker from '../';

export default { title: 'Components|AnglePicker', component: AnglePicker };

const AnglePickerWithState = () => {
const [ angle, setAngle ] = useState();
return (
<AnglePicker value={ angle } onChange={ setAngle } />
);
};

export const _default = () => {
return <AnglePickerWithState />;
};
42 changes: 42 additions & 0 deletions packages/components/src/angle-picker/style.scss
Original file line number Diff line number Diff line change
@@ -0,0 +1,42 @@
.components-angle-picker {
width: 50%;
&.components-base-control .components-base-control__label {
display: block;
}
}

.components-angle-picker__input-field {
width: calc(100% - #{$icon-button-size});
max-width: 100px;
}

.components-angle-picker__angle-circle {
width: $icon-button-size - ( 2 * $grid-size-small );
height: $icon-button-size - ( 2 * $grid-size-small );
border: 2px solid $dark-gray-500;
border-radius: 50%;
float: left;
margin-right: $grid-size-small;
cursor: grab;
}

.components-angle-picker__angle-circle-indicator-wrapper {
position: relative;
width: 100%;
height: 100%;
}

.components-angle-picker__angle-circle-indicator {
width: 1px;
height: 1px;
border-radius: 50%;
border: 3px solid $dark-gray-500;
display: block;
position: absolute;
top: -($icon-button-size - (2 * $grid-size-small)) / 2;
bottom: 0;
left: 0;
right: 0;
margin: auto;
background: $dark-gray-500;
}
1 change: 1 addition & 0 deletions packages/components/src/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ export { SVG, Path, Circle, Polygon, Rect, G, HorizontalRule, BlockQuotation } f

// Components
export { default as Animate } from './animate';
export { default as __experimentalAnglePicker } from './angle-picker';
export { default as Autocomplete } from './autocomplete';
export { default as BaseControl } from './base-control';
export { default as Button } from './button';
Expand Down
1 change: 1 addition & 0 deletions packages/components/src/style.scss
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
@import "./animate/style.scss";
@import "./angle-picker/style.scss";
@import "./autocomplete/style.scss";
@import "./base-control/style.scss";
@import "./button-group/style.scss";
Expand Down
88 changes: 88 additions & 0 deletions packages/compose/src/hooks/use-dragging/README.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,88 @@
`useDragging`
==============

In some situations, we want to have simple drag & drop behaviors.
Typically drag & drop behaviors follow a common pattern: We have an element that we want to drag or where we want dragging to start; the dragging starts when the `onMouseDown` event happens on the target element. When the dragging starts, global event listeners for mouse movement (`mousemove`) and the mouse up event (`mouseup`) are added. When the global mouse movement event triggers, the dragging behavior happens (e.g., a position is updated), when the mouse up event triggers, dragging stops, and both global event listeners are removed.
`useDragging` makes the implementation of the described common pattern simpler because it handles the addition and removal of global events.

## Input Object Properties

### `onDragStart`

- Type: `Function`

The hook calls `onDragStart` when the dragging starts. The function receives as parameters the same parameters passed to `startDrag` whose documentation is available below.
If `startDrag` is passed directly as an `onMouseDown` event handler, `onDragStart` will receive the `onMouseDown` event.

### `onDragMove`

- Type: `Function`

The hook calls `onDragMove ` after the dragging starts and when a mouse movement happens.
It receives the `mousemove` event.

### `onDragEnd`

- Type: `Function`

The hook calls `onDragEnd` when the dragging ends. When dragging is explicitly stopped, the function receives as parameters, the same parameters passed to `endDrag` whose documentation is available below.
When dragging stops because the user releases the mouse, the function receives the `mouseup` event.

## Return Object Properties

### `startDrag`

- Type: `Function`

A function that, when called, starts the dragging behavior. Parameters passed to this function will be passed to `onDragStart` when the dragging starts.
It is possible to directly pass `startDrag` as the `onMouseDown` event handler of some element.

### `endDrag`

- Type: `Function`

A function that, when called, stops the dragging behavior. Parameters passed to this function will be passed to `onDragEnd` when the dragging ends.
In most cases, there is no need to call this function directly. Dragging behavior automatically stops when the mouse is released.

### `isDragging`

- Type: `Boolean`

A boolean value, when true it means dragging is currently taking place; when false, it means dragging is not taking place.

## Usage
The following example allows us to drag & drop a red square around the entire viewport.

```jsx
/**
* WordPress dependencies
*/
import { useState, useCallback } from '@wordpress/element';
import { __experimentalUseDragging as useDragging } from '@wordpress/compose';


const UseDraggingExample = () => {
const [ position, setPosition ] = useState( null );
const changePosition = useCallback(
( event ) => {
setPosition( { x: event.clientX, y: event.clientY } );
}
);
const { startDrag } = useDragging( {
onDragMove: changePosition,
} );
return (
// eslint-disable-next-line jsx-a11y/no-static-element-interactions
<div
onMouseDown={ startDrag }
style={ {
position: 'fixed',
width: 10,
height: 10,
backgroundColor: 'red',
...( position ? { top: position.y, left: position.x } : {} ),
} }
/>
);
};
```
74 changes: 74 additions & 0 deletions packages/compose/src/hooks/use-dragging/index.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,74 @@
/**
* WordPress dependencies
*/
import {
useCallback,
useEffect,
useLayoutEffect,
useRef,
useState,
} from '@wordpress/element';

const useIsomorphicLayoutEffect =
typeof window !== 'undefined' ? useLayoutEffect : useEffect;
Copy link
Member

Choose a reason for hiding this comment

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

@jorgefilipecosta any context for why we introduced this? It's unclear to me what this is supposed to do, especially since the React docs mention that neither useLayoutEffect nor useEffect can run until downloading JS in the browser.

Copy link
Member Author

Choose a reason for hiding this comment

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

Hi @dmsnell, if we use useLayoutEffect on a component and that component is server-side rendered or rendered to a string using renderToString there will be a warning/error. So we use useIsomorphicLayoutEffect to try to make it possible to have our components used in projects relying on server-side render.

Copy link
Member

Choose a reason for hiding this comment

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

thanks for the clarification @jorgefilipecosta

two follow-ups:

  • why swap out useEffect since according to the React docs it has the same issues on server render as useLayoutEffect does, namely that it won't run on the server because they have to wait until the browser has loaded and parsed the JS.

  • why hide the warning? isn't that there to alert developers that their components won't likely behave the way they think if rendered from the server? am I misunderstanding or are we simply hiding that warning from people while preserving the same risk?

Copy link
Member Author

Choose a reason for hiding this comment

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

Well, I think in this case if we server-side render a component even if the effect does not run the component renders the expected HTML just without the events so hiding the warning is a good thing.
But, I remember I explored at the time some open source code for similar situations/event handling and the useIsomorphicLayoutEffect was common practice so I just followed it, if you think it is unnecessary in this case we can remove it.

Copy link
Member

Choose a reason for hiding this comment

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

if you think it is unnecessary in this case we can remove it.

Might do that if we can be reasonably sure it's not a depended-upon API.
The way I think through it I think I'd rather leave that warning to alert people of the potential issues, which I think are basically that if you perform any state mutation in those effects that you expect to appear on the first render then they won't be there on server renders, and that's true for both functions, so I am confused why we make the switch based on whether window exists, unless again it's just to hide the warning.

After a bit of searching I'm finding the whole topic a big ambiguous in the official docs and declarations. It appears like the only reason useEffect doesn't throw a warning on server render is more happenstance than intent: it simply never runs on the server and so doesn't get a chance to complain.

to make it possible to have our components used in projects relying on server-side render

what prevents someone from using this in server-side render context if it spits out a warning?

Copy link
Member Author

Choose a reason for hiding this comment

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

Might do that if we can be reasonably sure it's not a depended-upon API.

On this specific component, useIsomorphicLayoutEffect was not exposed and we can safely not use it. But it seems after this work a useIsomorphicLayoutEffect hook was added and was exposed. We can remove the use cases of the hook when it makes sense.


export default function useDragging( { onDragStart, onDragMove, onDragEnd } ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we add some docs for this hook, it's not clear to me how it's supposed to work? Instead of these callbacks, should it receive a ref instead?

Copy link
Member Author

Choose a reason for hiding this comment

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

Hi @youknowriad, I added a readme file as suggested.

const [ isDragging, setIsDragging ] = useState( false );

const eventsRef = useRef( {
onDragStart,
onDragMove,
onDragEnd,
} );
useIsomorphicLayoutEffect(
() => {
eventsRef.current.onDragStart = onDragStart;
eventsRef.current.onDragMove = onDragMove;
eventsRef.current.onDragEnd = onDragEnd;
},
[ onDragStart, onDragMove, onDragEnd ]
);

const onMouseMove = useCallback(
( ...args ) => ( eventsRef.current.onDragMove && eventsRef.current.onDragMove( ...args ) ),
[]
);
const endDrag = useCallback(
( ...args ) => {
if ( eventsRef.current.onDragEnd ) {
eventsRef.current.onDragEnd( ...args );
}
document.removeEventListener( 'mousemove', onMouseMove );
document.removeEventListener( 'mouseup', endDrag );
setIsDragging( false );
},
[]
);
const startDrag = useCallback(
( ...args ) => {
if ( eventsRef.current.onDragStart ) {
eventsRef.current.onDragStart( ...args );
}
document.addEventListener( 'mousemove', onMouseMove );
document.addEventListener( 'mouseup', endDrag );
setIsDragging( true );
},
[]
);

// Remove the global events when unmounting if needed.
useEffect( () => {
return () => {
if ( isDragging ) {
document.removeEventListener( 'mousemove', onMouseMove );
document.removeEventListener( 'mouseup', endDrag );
}
};
}, [ isDragging ] );

return {
startDrag,
endDrag,
isDragging,
};
}
1 change: 1 addition & 0 deletions packages/compose/src/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ export { default as withSafeTimeout } from './higher-order/with-safe-timeout';
export { default as withState } from './higher-order/with-state';

// Hooks
export { default as __experimentalUseDragging } from './hooks/use-dragging';
export { default as useInstanceId } from './hooks/use-instance-id';
export { default as useKeyboardShortcut } from './hooks/use-keyboard-shortcut';
export { default as useMediaQuery } from './hooks/use-media-query';
Expand Down
Loading