BoxControl: Convert to TypeScript #47622
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Size Change: +2.15 kB (0%) Total Size: 1.31 MB
ℹ️ View Unchanged
|
mirka
commented
Jan 31, 2023
Comment on lines
51
to
55
| Props for the internal [UnitControl](../unit-control) components. | ||
|
|
||
| - Type: `Object` | ||
| - Required: No | ||
| - Default: `{ min: 0 }` |
There was a problem hiding this comment.
These props are actually passed through to UnitControl, not InputControl. Why does it matter? InputControlProps[ 'max' ] is string but UnitControlProps[ 'max' ] is number ☠️
There was a problem hiding this comment.
😵💫 Hopefully this will be progressively addressed as we work our way up from NumberControl
| const allValue = getAllValue( values, selectedUnits, sides ); | ||
| const hasValues = isValuesDefined( values ); | ||
| const isMixed = hasValues && isValuesMixed( values, selectedUnits, sides ); | ||
| const allPlaceholder = isMixed ? LABELS.mixed : null; | ||
| const allPlaceholder = isMixed ? LABELS.mixed : undefined; |
There was a problem hiding this comment.
The placeholder prop on inputs do not accept null.
| const handleOnChange = ( next ) => { | ||
| const isNumeric = ! isNaN( parseFloat( next ) ); | ||
| const handleOnChange: UnitControlProps[ 'onChange' ] = ( next ) => { | ||
| const isNumeric = next !== undefined && ! isNaN( parseFloat( next ) ); |
There was a problem hiding this comment.
Just making the undefined case explicit. Should not change runtime behavior.
| @@ -132,7 +166,7 @@ export default function BoxControl( { | |||
| <FlexItem> | |||
| <Button | |||
| className="component-box-control__reset-button" | |||
| isSecondary | |||
| variant="secondary" | |||
There was a problem hiding this comment.
Updated a deprecated prop.
| export function isValuesMixed( values = {}, selectedUnits, sides = ALL_SIDES ) { | ||
| export function isValuesMixed( | ||
| values: BoxControlValue = {}, | ||
| selectedUnits?: BoxControlValue, |
There was a problem hiding this comment.
I marked some of these as optional to match how the functions were actually being used in the code. For example:
There was a problem hiding this comment.
Great to see the BoxControl getting the TypeScript treatment. Nice work @mirka 👍
This is testing pretty well for me.
✅ No typing errors
✅ Unit tests pass
✅ BoxControl functions well in the Storybook
✅ No issues were spotted while smoke testing in the editors
I just have a couple of minor nitpick comments and questions.
❓ There are a few linting issues now in the utils.ts file.
❓ Should we have a description for the id property in the BoxControlProps for completeness
❓ Some of the prop descriptions differ between the types and the readme e.g. label, values
What's our preferred approach to our component readmes? Should they only outline the props for that component specifically or also all the ones that are passed through to an inner component, e.g. UnitControl?
Previously, I was also advised we are trying to update the formatting of our readmes when we convert components to TypeScript. Do we need to update the readme props to the following format?
### `propName`: `type`
Prop description.
- Required: No
- Default: `value`Example diff to fix linting errors
diff --git a/packages/components/src/box-control/types.ts b/packages/components/src/box-control/types.ts
index 541d1f3a5d..022334d948 100644
--- a/packages/components/src/box-control/types.ts
+++ b/packages/components/src/box-control/types.ts
@@ -73,7 +73,7 @@ export type BoxControlInputControlProps = UnitControlPassthroughProps & {
onChange?: ( nextValues: BoxControlValue ) => void;
onFocus?: (
_event: React.FocusEvent< HTMLInputElement >,
- { side: nextSide }: { side: keyof typeof LABELS }
+ { side }: { side: keyof typeof LABELS }
) => void;
onHoverOff?: (
sides: Partial< Record< keyof BoxControlValue, boolean > >
diff --git a/packages/components/src/box-control/utils.ts b/packages/components/src/box-control/utils.ts
index e4d2b23a1a..d28cc85f36 100644
--- a/packages/components/src/box-control/utils.ts
+++ b/packages/components/src/box-control/utils.ts
@@ -33,7 +33,7 @@ export const ALL_SIDES = [ 'top', 'right', 'bottom', 'left' ] as const;
* Gets an items with the most occurrence within an array
* https://stackoverflow.com/a/20762713
*
- * @param arr Array of items to check.
+ * @param arr Array of items to check.
* @return The item with the most occurrences.
*/
function mode< T >( arr: T[] ) {
@@ -49,9 +49,9 @@ function mode< T >( arr: T[] ) {
/**
* Gets the 'all' input value and unit from values data.
*
- * @param values Box values.
- * @param selectedUnits Box units.
- * @param availableSides Available box sides to evaluate.
+ * @param values Box values.
+ * @param selectedUnits Box units.
+ * @param availableSides Available box sides to evaluate.
*
* @return A value + unit for the 'all' input.
*/
@@ -102,8 +102,8 @@ export function getAllValue(
/**
* Determine the most common unit selection to use as a fallback option.
*
- * @param selectedUnits Current unit selections for individual sides.
- * @return Most common unit selection.
+ * @param selectedUnits Current unit selections for individual sides.
+ * @return Most common unit selection.
*/
export function getAllUnitFallback( selectedUnits?: BoxControlValue ) {
if ( ! selectedUnits || typeof selectedUnits !== 'object' ) {
@@ -118,9 +118,9 @@ export function getAllUnitFallback( selectedUnits?: BoxControlValue ) {
/**
* Checks to determine if values are mixed.
*
- * @param values Box values.
- * @param selectedUnits Box units.
- * @param sides Available box sides to evaluate.
+ * @param values Box values.
+ * @param selectedUnits Box units.
+ * @param sides Available box sides to evaluate.
*
* @return Whether values are mixed.
*/
@@ -138,9 +138,9 @@ export function isValuesMixed(
/**
* Checks to determine if values are defined.
*
- * @param values Box values.
+ * @param values Box values.
*
- * @return Whether values are mixed.
+ * @return Whether values are mixed.
*/
export function isValuesDefined( values?: BoxControlValue ) {
return (
@@ -158,8 +158,8 @@ export function isValuesDefined( values?: BoxControlValue ) {
* Get initial selected side, factoring in whether the sides are linked,
* and whether the vertical / horizontal directions are grouped via splitOnAxis.
*
- * @param isLinked Whether the box control's fields are linked.
- * @param splitOnAxis Whether splitting by horizontal or vertical axis.
+ * @param isLinked Whether the box control's fields are linked.
+ * @param splitOnAxis Whether splitting by horizontal or vertical axis.
* @return The initial side.
*/
export function getInitialSide( isLinked: boolean, splitOnAxis: boolean ) {
@@ -178,7 +178,7 @@ export function getInitialSide( isLinked: boolean, splitOnAxis: boolean ) {
* to their appropriate sides to facilitate correctly determining value for
* all input control.
*
- * @param sides Available sides for box control.
+ * @param sides Available sides for box control.
* @return Normalized sides configuration.
*/
export function normalizeSides( sides: BoxControlProps[ 'sides' ] ) {
@@ -204,9 +204,9 @@ export function normalizeSides( sides: BoxControlProps[ 'sides' ] ) {
* Applies a value to an object representing top, right, bottom and left sides
* while taking into account any custom side configuration.
*
- * @param currentValues The current values for each side.
- * @param newValue The value to apply to the sides object.
- * @param sides Array defining valid sides.
+ * @param currentValues The current values for each side.
+ * @param newValue The value to apply to the sides object.
+ * @param sides Array defining valid sides.
*
* @return Object containing the updated values for each side.
*/
|
Thanks for the review! All feedback has been addresssed 👍
Interesting, seems like my local linting rules were outdated and were enforcing a slightly different alignment. An
Addressed ✅
At this point I'm more invested in improving the TS as the source of truth, so we can eventually autogenerate the README props table from the TS data like we do in Storybook. With that in mind, I don't think it's particularly worthwhile to add more things to the READMEs, aside from things that are clearly necessary or fixing blatant inaccuracies.
Done ✅ |
|
Flaky tests detected in 1b1d1e5. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/4075613185
|
aaronrobertshaw
approved these changes
Feb 2, 2023
There was a problem hiding this comment.
Thanks for the quick turnaround and explanations on this one @mirka 🚀
LGTM! 🚢
✅ No typing errors or linting issues
✅ Unit tests pass
✅ BoxControl functions well in the Storybook
✅ No issues were spotted while smoke testing in the editors
✅ Readme and types are consistent
ciampo
reviewed
Feb 2, 2023
Comment on lines
58
to
60
| // @ts-expect-error - event.altKey is only present when the change event was | ||
| // triggered by a keyboard event. | ||
| if ( event.altKey ) { |
There was a problem hiding this comment.
Not sure it's a better solution, but we could avoid the @ts-expect-error by adding a typecast (and a runtime check for good measure?)
Suggested change
| // @ts-expect-error - event.altKey is only present when the change event was | |
| // triggered by a keyboard event. | |
| if ( event.altKey ) { | |
| if ( | |
| event.nativeEvent instanceof KeyboardEvent && | |
| ( event as React.KeyboardEvent ).altKey | |
| ) { |
haven't tested it at runtime, though
There was a problem hiding this comment.
I'll add a TODO comment here because I think it should be addressed in a different layer, for example in how the upstream change handler passes events. c64c2a6
|
|
||
| if ( ! sides?.length ) { | ||
| return ALL_SIDES; | ||
| } | ||
|
|
||
| if ( sides.includes( 'vertical' ) ) { | ||
| filteredSides.push( ...[ 'top', 'bottom' ] ); | ||
| filteredSides.push( ...( [ 'top', 'bottom' ] as const ) ); |
There was a problem hiding this comment.
cc @chad1008 this is an example of how as const can be used to tell the TypeScript compiler to narrow types as much as possible.
Without as const, TypeScript would assume string[] — which wouldn't be compatible with the type of filteredSides
|
|
||
| /** | ||
| * Gets an items with the most occurrence within an array | ||
| * https://stackoverflow.com/a/20762713 | ||
| * | ||
| * @param {Array<any>} arr Array of items to check. | ||
| * @return {any} The item with the most occurrences. | ||
| * @param arr Array of items to check. |
There was a problem hiding this comment.
Eslint auto-corrected the spacing in this file's JSDocs 🤷 eslint(jsdoc/check-line-alignment)
Not sure if my linter config is too strict?
diff --git a/packages/components/src/box-control/utils.ts b/packages/components/src/box-control/utils.ts
index 02bb728abd..e4d2b23a1a 100644
--- a/packages/components/src/box-control/utils.ts
+++ b/packages/components/src/box-control/utils.ts
@@ -33,7 +33,7 @@ export const ALL_SIDES = [ 'top', 'right', 'bottom', 'left' ] as const;
* Gets an items with the most occurrence within an array
* https://stackoverflow.com/a/20762713
*
- * @param arr Array of items to check.
+ * @param arr Array of items to check.
* @return The item with the most occurrences.
*/
function mode< T >( arr: T[] ) {
@@ -49,9 +49,9 @@ function mode< T >( arr: T[] ) {
/**
* Gets the 'all' input value and unit from values data.
*
- * @param values Box values.
- * @param selectedUnits Box units.
- * @param availableSides Available box sides to evaluate.
+ * @param values Box values.
+ * @param selectedUnits Box units.
+ * @param availableSides Available box sides to evaluate.
*
* @return A value + unit for the 'all' input.
*/
@@ -102,7 +102,7 @@ export function getAllValue(
/**
* Determine the most common unit selection to use as a fallback option.
*
- * @param selectedUnits Current unit selections for individual sides.
+ * @param selectedUnits Current unit selections for individual sides.
* @return Most common unit selection.
*/
export function getAllUnitFallback( selectedUnits?: BoxControlValue ) {
@@ -118,9 +118,9 @@ export function getAllUnitFallback( selectedUnits?: BoxControlValue ) {
/**
* Checks to determine if values are mixed.
*
- * @param values Box values.
- * @param selectedUnits Box units.
- * @param sides Available box sides to evaluate.
+ * @param values Box values.
+ * @param selectedUnits Box units.
+ * @param sides Available box sides to evaluate.
*
* @return Whether values are mixed.
*/
@@ -138,7 +138,7 @@ export function isValuesMixed(
/**
* Checks to determine if values are defined.
*
- * @param values Box values.
+ * @param values Box values.
*
* @return Whether values are mixed.
*/
@@ -158,8 +158,8 @@ export function isValuesDefined( values?: BoxControlValue ) {
* Get initial selected side, factoring in whether the sides are linked,
* and whether the vertical / horizontal directions are grouped via splitOnAxis.
*
- * @param isLinked Whether the box control's fields are linked.
- * @param splitOnAxis Whether splitting by horizontal or vertical axis.
+ * @param isLinked Whether the box control's fields are linked.
+ * @param splitOnAxis Whether splitting by horizontal or vertical axis.
* @return The initial side.
*/
export function getInitialSide( isLinked: boolean, splitOnAxis: boolean ) {
@@ -178,7 +178,7 @@ export function getInitialSide( isLinked: boolean, splitOnAxis: boolean ) {
* to their appropriate sides to facilitate correctly determining value for
* all input control.
*
- * @param sides Available sides for box control.
+ * @param sides Available sides for box control.
* @return Normalized sides configuration.
*/
export function normalizeSides( sides: BoxControlProps[ 'sides' ] ) {
@@ -204,9 +204,9 @@ export function normalizeSides( sides: BoxControlProps[ 'sides' ] ) {
* Applies a value to an object representing top, right, bottom and left sides
* while taking into account any custom side configuration.
*
- * @param currentValues The current values for each side.
- * @param newValue The value to apply to the sides object.
- * @param sides Array defining valid sides.
+ * @param currentValues The current values for each side.
+ * @param newValue The value to apply to the sides object.
+ * @param sides Array defining valid sides.
*
* @return Object containing the updated values for each side.
*/There was a problem hiding this comment.
Reading this previous answer, I made sure to npm ci from latest trunk to make sure that the linter rules are up to date, and I still got the same diff as in my previous message.
Maybe @aaronrobertshaw 's local lint rules were slightly out of date?
I guess the "final" check could be to rebase this PR on top of latest trunk, re-run npm ci and see what the linter on @mirka 's machine outputs?
There was a problem hiding this comment.
Mmm, still the same after an npm ci with trunk and npm run lint:js -- packages/components/src/box-control/utils.ts 🤔 No box-control/ warnings on CI either. In any case there are a lot of existing alignment warnings, and since they are only warnings, I expect they will continue to flip-flop for a while 😅
DaisyOlsen
added
the
[Type] Code Quality
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Part of #35744
What?
Converts the BoxControl component to TypeScript, including tests.
📌 To cut down on scope, Storybook stories were not converted in this PR.
How?
See inline comments for details.
Testing Instructions
npm run storybook:devand see the Docs view for BoxControl.