Refactor FocalPointPicker to function component
#39168
Conversation
stokesman
added
[Type] Code Quality
|
Size Change: -834 B (0%) Total Size: 1.24 MB
ℹ️ View Unchanged
|
stokesman
force-pushed
the
refactor/focal-point-picker-fn
branch
from
1313547
to
b6d4df3
Compare
stokesman
force-pushed
the
refactor/focal-point-picker-fn
branch
2 times, most recently
from
8163f00
to
f38772b
Compare
| percentages = { | ||
| point = { | ||
| x: 0.5, | ||
| y: 0.5, | ||
| }, | ||
| } ) { | ||
| const valueX = fractionToPercentage( percentages.x ); | ||
| const valueY = fractionToPercentage( percentages.y ); | ||
| const valueX = fractionToPercentage( point.x ); | ||
| const valueY = fractionToPercentage( point.y ); | ||
|
|
||
| const handleChange = ( value, axis ) => { | ||
| const num = parseInt( value, 10 ); | ||
|
|
||
| if ( ! isNaN( num ) ) { | ||
| onChange( { ...percentages, [ axis ]: num / 100 } ); | ||
| onChange( { ...point, [ axis ]: num / 100 } ); |
There was a problem hiding this comment.
This is cosmetic but point just seems like a better name than percentages. It's an internal component so there's no concern of breakage.
| export default function FocalPoint( { | ||
| coordinates = { left: '50%', top: '50%' }, | ||
| ...props | ||
| } ) { | ||
| export default function FocalPoint( { left = '50%', top = '50%', ...props } ) { | ||
| const classes = classnames( | ||
| 'components-focal-point-picker__icon_container' | ||
| ); | ||
|
|
||
| const style = { | ||
| left: coordinates.left, | ||
| top: coordinates.top, | ||
| }; | ||
| const style = { left, top }; |
There was a problem hiding this comment.
Cosmetic. The separate left and right props are a touch simpler. Less abstract seems a fine thing for an internal component.
| export default function FocalPointPickerGrid( { | ||
| bounds = {}, | ||
| value, | ||
| ...props | ||
| } ) { | ||
| export default function FocalPointPickerGrid( { bounds, value, ...props } ) { |
There was a problem hiding this comment.
bounds is always supplied by the parent component and does not need to default.
| onLoad = noop, | ||
| onLoad, |
There was a problem hiding this comment.
There is no need to default this. The parent component supplies it unconditionally.
| export const INITIAL_BOUNDS = { | ||
| top: 0, | ||
| left: 0, | ||
| bottom: 0, | ||
| right: 0, | ||
| width: 0, | ||
| height: 0, | ||
| width: 200, | ||
| height: 170, | ||
| }; |
There was a problem hiding this comment.
The width and height values are extracted from the styles and the styles now pull them from here. The component was also no longer in need of all the other properties.
stokesman
force-pushed
the
refactor/focal-point-picker-fn
branch
2 times, most recently
from
8854204
to
de2fd11
Compare
What I took from my experience reviewing that PR was: It's probably easier to review if the FC and TS conversions are done in separate PRs 😄 I just didn't think it was worth codifying in the contributing guide because there are only a few class components left. Is this ready for review, in that case? |
stokesman
force-pushed
the
refactor/focal-point-picker-fn
branch
from
a57bbef
to
7aedf1f
Compare
Now that I've linted with |
There was a problem hiding this comment.
Thank you @stokesman for working on this.
I had a look at the component on Storybook and found a few regressions:
- the coordinates reset when the component loses focus (i.e. clicking outside of the interactive area)
- the thirds grid in the "Video" example does not adapt to its container
With regards to the TS refactor, I would have probably worked on that first, in order to gain even more confidence when working on the follow-up refactor from class to functional component.
I would also like to express a personal preference for avoiding runtime changes as much as possible (ref optimizations, drag logic refactor, variable name changes...) during "refactor" PRs like this one, as they increase considerably the difficulty amount of time required to review the PR. Those are all great improvements, but I would personally prefer if they were applied in one or more follow-up PRs, keeping the original PR focused on the refactor from class to function component.
| if ( byTenths ) { | ||
| nextX = Math.round( nextX / 0.1 ) * 0.1; | ||
| nextY = Math.round( nextY / 0.1 ) * 0.1; | ||
| } |
There was a problem hiding this comment.
With this change, the values are not clamped anymore if byTenths is false — what's reason behind it? Why do are we moving away from roundClamp ? (reviewing this change in the context of the first "cleanup" commit in this PR)
There was a problem hiding this comment.
The values are clamped a few lines before this making the clamp of roundClamp redundant.
The larger story is one of yak-shaving. This PR got started when working on another PR (#34566) and wanting to modify roundClamp for a more specific purpose. I noticed the only other place it was used was this component, spotted the partial redundancy and figured it could be removed here to potentially free it up for the repurposing.
There was a problem hiding this comment.
Makes sense. I'm going to keep this conversation open for extra visibility in case anyone else reviews this PR
🤦 That was introduced with 832ea2c and now fixed with 3d8d486
Great catch. Fixed it in 976b4b3. Sort of makes me think we may want a story with an intentionally 404'd media URL because otherwise we might have missed this. I don't believe a unit test could cover it because of jsdom and lack of element dimensions. |
There was a problem hiding this comment.
Most of my first round of feedback has been addressed, and the Storybook example seems to work as expected.
Given the fact that the code changes in this PR are not trivial, I've also tagged @mirka in case she wants to take a look as well before merging.
| if ( byTenths ) { | ||
| nextX = Math.round( nextX / 0.1 ) * 0.1; | ||
| nextY = Math.round( nextY / 0.1 ) * 0.1; | ||
| } |
There was a problem hiding this comment.
Makes sense. I'm going to keep this conversation open for extra visibility in case anyone else reviews this PR
|
While giving this PR another round of testing, I discovered another bug (which is also present in Basically, Kapture.2022-07-20.at.12.03.38.mp4 |
There was a problem hiding this comment.
I don't think I've gone as deep as Marco did, but I read through the code and nothing major is jumping out 👍
One happy find is that I think this PR fixes a bug in trunk where the indicator offset gets weird after scrolling:
CleanShot.2022-07-22.at.02.54.33.mp4
| role="button" | ||
| tabIndex="-1" | ||
| > | ||
| <Grid bounds={ bounds } value={ `${ x }${ y }` } /> |
There was a problem hiding this comment.
(Just a comment, not meant to address in this PR:) I found it a bit strange that a prop named value is being used basically as a re-rendering key. I wonder if it would be more straightforward (and efficient) if we simply passed isDragging instead of every changed value.
There was a problem hiding this comment.
Yes, 💯 strange.
Your suggested solution seems great. It will require a little reworking of Grid but I'd expect it to be an all around improvement.
There was a problem hiding this comment.
I thought of something that might block the suggestion. Right now Grid makes itself visible not just when dragging but also during any value changes (arrow keys while the drag area is focused or changes at the number inputs). There'd be no way to maintain that by merely using isDragging.
Maybe just changing the prop from value to key would be the way to go.
There was a problem hiding this comment.
We could simplify the code by passing a showOverlay prop to the Grid component, moving the timeout logic to index.js
Click to expand
diff --git a/packages/components/src/focal-point-picker/grid.js b/packages/components/src/focal-point-picker/grid.js
index ecd6d6b7d9..ed6d8ae51d 100644
--- a/packages/components/src/focal-point-picker/grid.js
+++ b/packages/components/src/focal-point-picker/grid.js
@@ -1,8 +1,3 @@
-/**
- * WordPress dependencies
- */
-import { useState } from '@wordpress/element';
-
/**
* Internal dependencies
*/
@@ -11,21 +6,16 @@ import {
GridLineX,
GridLineY,
} from './styles/focal-point-picker-style';
-import { useUpdateEffect } from '../utils/hooks';
-
-export default function FocalPointPickerGrid( { bounds, value, ...props } ) {
- const animationProps = useRevealAnimation( value );
- const style = {
- width: bounds.width,
- height: bounds.height,
- };
+export default function FocalPointPickerGrid( { bounds, ...props } ) {
return (
<GridView
{ ...props }
- { ...animationProps }
className="components-focal-point-picker__grid"
- style={ style }
+ style={ {
+ width: bounds.width,
+ height: bounds.height,
+ } }
>
<GridLineX style={ { top: '33%' } } />
<GridLineX style={ { top: '66%' } } />
@@ -34,25 +24,3 @@ export default function FocalPointPickerGrid( { bounds, value, ...props } ) {
</GridView>
);
}
-
-/**
- * Custom hook that renders the "flash" animation whenever the value changes.
- *
- * @param {string} value Value of (box) side.
- */
-function useRevealAnimation( value ) {
- const [ isActive, setIsActive ] = useState( false );
-
- useUpdateEffect( () => {
- setIsActive( true );
- const timeout = window.setTimeout( () => {
- setIsActive( false );
- }, 600 );
-
- return () => window.clearTimeout( timeout );
- }, [ value ] );
-
- return {
- isActive,
- };
-}
diff --git a/packages/components/src/focal-point-picker/index.js b/packages/components/src/focal-point-picker/index.js
index 09128c04b9..5c1f594641 100644
--- a/packages/components/src/focal-point-picker/index.js
+++ b/packages/components/src/focal-point-picker/index.js
@@ -27,6 +27,9 @@ import {
MediaContainer,
} from './styles/focal-point-picker-style';
import { INITIAL_BOUNDS } from './utils';
+import { useUpdateEffect } from '../utils/hooks';
+
+const GRID_OVERLAY_TIMEOUT = 600;
export default function FocalPointPicker( {
autoPlay = true,
@@ -45,6 +48,7 @@ export default function FocalPointPicker( {
},
} ) {
const [ point, setPoint ] = useState( valueProp );
+ const [ showGridOverlay, setShowGridOverlay ] = useState( false );
const { startDrag, endDrag, isDragging } = useDragging( {
onDragStart: ( event ) => {
@@ -146,6 +150,15 @@ export default function FocalPointPicker( {
const instanceId = useInstanceId( FocalPointPicker );
const id = `inspector-focal-point-picker-control-${ instanceId }`;
+ useUpdateEffect( () => {
+ setShowGridOverlay( true );
+ const timeout = window.setTimeout( () => {
+ setShowGridOverlay( false );
+ }, GRID_OVERLAY_TIMEOUT );
+
+ return () => window.clearTimeout( timeout );
+ }, [ x, y ] );
+
return (
<BaseControl
label={ label }
@@ -165,7 +178,7 @@ export default function FocalPointPicker( {
role="button"
tabIndex="-1"
>
- <Grid bounds={ bounds } value={ `${ x }${ y }` } />
+ <Grid bounds={ bounds } showOverlay={ showGridOverlay } />
<Media
alt={ __( 'Media preview' ) }
autoPlay={ autoPlay }
diff --git a/packages/components/src/focal-point-picker/styles/focal-point-picker-style.js b/packages/components/src/focal-point-picker/styles/focal-point-picker-style.js
index 0687275c7f..58fa3eb5de 100644
--- a/packages/components/src/focal-point-picker/styles/focal-point-picker-style.js
+++ b/packages/components/src/focal-point-picker/styles/focal-point-picker-style.js
@@ -61,7 +61,6 @@ export const ControlWrapper = styled( Flex )`
export const GridView = styled.div`
left: 50%;
- opacity: 0;
overflow: hidden;
pointer-events: none;
position: absolute;
@@ -70,11 +69,7 @@ export const GridView = styled.div`
transition: opacity 120ms linear;
z-index: 1;
- ${ ( { isActive } ) =>
- isActive &&
- `
- opacity: 1;
- ` }
+ opacity: ${ ( { showOverlay } ) => ( showOverlay ? 1 : 0 ) };
`;
export const GridLine = styled.div`There was a problem hiding this comment.
Works for me! I applied it in 4f99530. I also tested again in Storybook and the Post Editor and found it works as expected.
I vaguely remember encountering that and I definitely remember simplifying the logic to find the proper offset. Yet I also think that issue is only in Storybook for some reason. Maybe the iframe? So perhaps it could happen in the Site Editor as well (EDIT: probably not because I think only the canvas is in the iframe and not the inspector controls). I can't remember how much I looked into that. |
|
Hey @stokesman , do you think you'll be able to finish the work on this one? |
stokesman
force-pushed
the
refactor/focal-point-picker-fn
branch
2 times, most recently
from
db440cd
to
e0015da
Compare
Eventually 😄 Though, where I'd left off there were no requested changes and it seemed like a changelog was all that was required. Since then a couple merges introduced conflicts and I've rebased and resolved those. I also added a commit regarding the conversation Lena started #39168 (comment). |
|
Thank you, Mitchell! I gave another round of test in Storybook and the component is looking and behaving well! Once #39168 (comment) is addressed and a CHANGELOG entry is added, we should be able to merge it! |
Co-authored-by: Marco Ciampini <1083581+ciampo@users.noreply.github.com>
stokesman
force-pushed
the
refactor/focal-point-picker-fn
branch
from
e0015da
to
3b88aa4
Compare
|
Thanks for testing and reviewing Marco and Lena 🙇 |
Part of #22890.
When starting on this I first looked for some small things that could be simplified and put them in the first commit since they would be hard to spot within the rewrite as a function component. Once I got into the rewrite, I found more things to simplify:
useDraggingfrom the compose package.Testing Instructions
Make sure the
FocalPointPickerworks as expected. Especially in the Media & Text block and Cover blocks.Screenshots
Types of changes
Refactor
Checklist:
*.native.jsfiles for terms that need renaming or removal).