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

FocalPointPicker: Allow updating its value while dragging. #38247

Merged
merged 20 commits into from
Feb 10, 2022
Merged

FocalPointPicker: Allow updating its value while dragging. #38247

Show file tree
Hide file tree
Changes from 17 commits
Commits
Show all changes
20 commits
Select commit Hold shift + click to select a range
8705154
Update values of focal point picker with incoming props even if user …
razwan Jan 26, 2022
f41d120
Merge branch 'trunk' into update/focal-point-picker-incoming-props
razwan Feb 1, 2022
467f3a3
Update current value of focal point picker with value returned by onD…
razwan Feb 1, 2022
5ed3b2e
Use resolvePoint prop to update focal point picker value instead of r…
razwan Feb 8, 2022
4aac1f6
Call onDrag with the maybe modified value returned by resolvePoint
razwan Feb 8, 2022
95cb4b4
Add docs for the recently added resolvePoint prop
razwan Feb 8, 2022
3119b56
Reverse order of updateValue and onDrag to be the same as before addi…
razwan Feb 8, 2022
54d8cc0
Call resolvePoint from updateValue so that even changes from the inpu…
razwan Feb 9, 2022
b82a001
Add storybook example of implementing snapping functionality for foca…
razwan Feb 9, 2022
7205351
Add mention of the resolvePoint prop addition in the changelog
razwan Feb 9, 2022
f74c789
Add unit test for resolvePoint prop
razwan Feb 9, 2022
541c658
Fix unit test for FocalPointPicker resolvePoint prop
razwan Feb 9, 2022
720a1c6
Fix typo in FocalPointPicker story
razwan Feb 9, 2022
518475a
Update resolvePoint description in FocalPointPicker readme
razwan Feb 9, 2022
6d62ff0
Better unit test for resolvePoint
razwan Feb 9, 2022
40f9e9e
Add callback to updateValue so onChange gets called with resolved val…
razwan Feb 9, 2022
3bd8847
Merge branch 'trunk' into update/focal-point-picker-incoming-props
razwan Feb 9, 2022
9ac9517
Move changelog entry to unreleased section
razwan Feb 10, 2022
9ea5d61
Add unit test to test onChange after keyboard interaction without res…
razwan Feb 10, 2022
38c6446
Merge branch 'trunk' into update/focal-point-picker-incoming-props
razwan Feb 10, 2022
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
3 changes: 2 additions & 1 deletion packages/components/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,8 @@

- Refine `ExternalLink` to be same size as the text, to appear more as a glyph than an icon. ([#37859](https://github.com/WordPress/gutenberg/pull/37859))
- Updated `ToolsPanel` header icon to only show "plus" icon when all items are optional and all are currently hidden ([#38262](https://github.com/WordPress/gutenberg/pull/38262))
- `TreeGrid`: Fix keyboard navigation for expand/collapse table rows in Firefox ([#37983](https://github.com/WordPress/gutenberg/pull/37983))
- `TreeGrid`: Fix keyboard navigation for expand/collapse table rows in Firefox ([#37983](https://github.com/WordPress/gutenberg/pull/37983))
- Add `resolvePoint` prop to `FocalPointPicker` to allow updating the value of the picker after a user interaction ([#38247](https://github.com/WordPress/gutenberg/pull/38247))

### Bug Fix

Expand Down
7 changes: 7 additions & 0 deletions packages/components/src/focal-point-picker/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,3 +94,10 @@ Callback which is called at the end of drag operations.
- Required: No

Callback which is called at the start of drag operations.

### `resolvePoint`

- Type: `Function`
- Required: No

Function which is called before internal updates to the value state. It receives the upcoming value and may return a modified one.
19 changes: 12 additions & 7 deletions packages/components/src/focal-point-picker/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,8 +49,9 @@ export class FocalPointPicker extends Component {
}
};
this.onChangeAtControls = ( value ) => {
this.updateValue( value );
this.props.onChange( value );
this.updateValue( value, () => {
this.props.onChange( this.state.percentages );
} );
};

this.updateBounds = this.updateBounds.bind( this );
Expand Down Expand Up @@ -128,15 +129,18 @@ export class FocalPointPicker extends Component {
}
return bounds;
}
updateValue( nextValue = {} ) {
const { x, y } = nextValue;
updateValue( nextValue = {}, callback ) {
const resolvedValue =
this.props.resolvePoint?.( nextValue ) ?? nextValue;

const { x, y } = resolvedValue;

const nextPercentage = {
x: parseFloat( x ).toFixed( 2 ),
y: parseFloat( y ).toFixed( 2 ),
};

this.setState( { percentages: nextPercentage } );
this.setState( { percentages: nextPercentage }, callback );
}
updateBounds() {
this.setState( {
Expand Down Expand Up @@ -180,8 +184,9 @@ export class FocalPointPicker extends Component {

next[ axis ] = roundClamp( value, 0, 1, step );

this.updateValue( next );
this.props.onChange( next );
this.updateValue( next, () => {
this.props.onChange( this.state.percentages );
} );
}
doDrag( event ) {
// Prevents text-selection when dragging.
Expand Down
30 changes: 30 additions & 0 deletions packages/components/src/focal-point-picker/stories/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,3 +44,33 @@ export const video = () => {

return <Example url={ url } />;
};

export const snapping = () => {
const snapValues = {
x: [ 0, 0.33, 0.66, 1 ],
y: [ 0, 0.33, 0.66, 1 ],
};

const threshold = 0.05;

const maybeSnapFocalPoint = ( value ) => {
let x = parseFloat( value.x );
let y = parseFloat( value.y );

snapValues.x.forEach( ( snapValue ) => {
if ( snapValue - threshold < x && x < snapValue + threshold ) {
x = snapValue;
}
} );

snapValues.y.forEach( ( snapValue ) => {
if ( snapValue - threshold < y && y < snapValue + threshold ) {
y = snapValue;
}
} );

return { x, y };
};

return <Example resolvePoint={ maybeSnapFocalPoint } />;
};
27 changes: 27 additions & 0 deletions packages/components/src/focal-point-picker/test/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,33 @@ describe( 'FocalPointPicker', () => {
} );
} );

describe( 'resolvePoint handling', () => {
it( 'should allow value altering', async () => {
const spyChange = jest.fn();
const spy = jest.fn();
const { getByRole } = render(
<Picker
value={ { x: 0.25, y: 0.25 } }
onChange={ spyChange }
resolvePoint={ () => {
spy();
return { x: 0.91, y: 0.42 };
} }
Comment on lines +73 to +76
Copy link
Contributor

Choose a reason for hiding this comment

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

Q: when checking this test on my local machine, I tried to make the test fail by modifying the resolvePoint function to

resolvePoint={ ( value ) => {
  spy();
  return value;
} }

The test fails (as expected), but the values received by the onChange spy are not x: 0.25, y: 0.25, but instead are x: 0, y: 0. I wasn't expecting this behaviour, do you know why would this happen?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My guess is that the main reason is that the picker doesn't have width and height props. Also, fireEvent is called without any other parameters but am not sure if the case would be the same if the picker had set dimensions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, so I haven't been able to use mouseMove to create a proper emulation of a user interaction. However, I managed that by using fireEvent.keyDown( dragArea, { charCode: 0, keyCode: 38 } ); which triggers onChange with an "expected" value of x: 0.25, y: 0.24 making my test fail. That was because onChange was called with the same value updateValue would received.

I added a second parameter to the updateValue method which is a callback that is passed to the setState call. Moving the onChange call inside that callback fixed the issue and made the test pass. Everything looks fine now.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thank you for the explanation, makes sense!

I suggest we add another unit test to check this onChange baheviour:

diff --git a/packages/components/src/focal-point-picker/test/index.js b/packages/components/src/focal-point-picker/test/index.js
index 145ca0d019..ac4f1af0e3 100644
--- a/packages/components/src/focal-point-picker/test/index.js
+++ b/packages/components/src/focal-point-picker/test/index.js
@@ -76,6 +76,8 @@ describe( 'FocalPointPicker', () => {
 					} }
 				/>
 			);
+
+			// Click and press arrow up
 			const dragArea = getByRole( 'button' );
 			act( () => {
 				fireEvent.mouseDown( dragArea );
@@ -98,5 +100,23 @@ describe( 'FocalPointPicker', () => {
 			rerender( <Picker value={ { x: 0.93, y: 0.5 } } /> );
 			expect( xInput.value ).toBe( '93' );
 		} );
+
+		it( 'call onChange with the expected values', async () => {
+			const spyChange = jest.fn();
+			const { getByRole } = render(
+				<Picker value={ { x: 0.14, y: 0.62 } } onChange={ spyChange } />
+			);
+
+			// Click and press arrow up
+			const dragArea = getByRole( 'button' );
+			act( () => {
+				fireEvent.mouseDown( dragArea );
+				fireEvent.keyDown( dragArea, { charCode: 0, keyCode: 38 } );
+			} );
+			expect( spyChange ).toHaveBeenCalledWith( {
+				x: '0.14',
+				y: '0.61',
+			} );
+		} );
 	} );
 } );

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure I understand what you're asking for. Doesn't the initial test cover this behaviour? Do you want me to remove the onChange part from that test and then make another one in the controllability section that only tests the onChange part?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm proposing to add another unit test aimed at testing the behaviour that I highlighted earlier and you fixed in your latest commits by using the setState callback to call onChange.

Earlier, when removing resolvePoint from this test, the onChange spy was receiving x: 0, y: 0 instead of the expected value. Thanks to your fix, the onChange would now receive the correct values. My proposal is to add an additional unit test to specifically test that (note that, in the new test, the resolvePoint prop is not specified)

Hope I managed to explain myself better :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll add the unit test but I want to make sure we're on the same page.

By using mouseDown and mouseMove we still receive { x: 0.00, y: 0.00 } instead of the values with which the picker is initialised.

The bug that was fixed by moving the onChange inside the setState callback was related to the resolvePoint prop.

this.updateValue( next ); // next was being passed to resolvePoint here
this.props.onChange( next ); // but not here

This unit test becomes a test to future proof the keyboard interaction (rather for the UP key press). Am I right?

Copy link
Contributor

Choose a reason for hiding this comment

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

By using mouseDown and mouseMove we still receive { x: 0.00, y: 0.00 } instead of the values with which the picker is initialised.

I see, I missed that detail. Do you think it's worth investigating this in a follow-up PR?

/>
);
const dragArea = getByRole( 'button' );
act( () => {
fireEvent.mouseDown( dragArea );
fireEvent.keyDown( dragArea, { charCode: 0, keyCode: 38 } );
} );
expect( spy ).toHaveBeenCalled();
expect( spyChange ).toHaveBeenCalledWith( {
x: '0.91',
y: '0.42',
} );
} );
} );

describe( 'controllability', () => {
it( 'should update value from props', () => {
const { rerender, getByRole } = render(
Expand Down