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

NumberControl: Add TypeScript prop types #43791

Merged
merged 21 commits into from
Sep 8, 2022
Merged

NumberControl: Add TypeScript prop types #43791

merged 21 commits into from
Sep 8, 2022

Conversation

mirka
Copy link
Member

@mirka mirka commented Sep 1, 2022

Part of #35744

What?

Adds "provisional" prop types to NumberControl.

Why?

In the past, we've had major complications with properly typing NumberControl due to the string/number ambiguity of the values both upstream and downstream. This is why NumberControl is untyped and @ts-nochecked at the moment.

However, this has started to bottleneck TypeScript migrations for other components that inherit NumberControl props. I think it would be useful to improve the situation a bit by actually defining prop types, even if there are some contradictions that need to be @ts-ignored.

I hope this reads as a constructive baby step forward, rather than a messy pile of bandaids. The benefit is that we can clearly demarcate where the type contradictions are, instead of keeping a huge untyped blackhole in our component chain.

How?

See inline code comments for minor runtime changes.

Testing Instructions

npm run storybook:dev and see the Docs view for NumberControl and UnitControl.

@mirka mirka added the [Package] Components /packages/components label Sep 1, 2022
@mirka mirka self-assigned this Sep 1, 2022
@github-actions
Copy link

github-actions bot commented Sep 1, 2022

Size Change: +2.84 kB (0%)

Total Size: 1.25 MB

Filename Size Change
build/block-editor/index.min.js 162 kB +858 B (+1%)
build/block-editor/style-rtl.css 15.2 kB +46 B (0%)
build/block-editor/style.css 15.2 kB +49 B (0%)
build/block-library/blocks/audio/theme-rtl.css 126 B +16 B (+15%) ⚠️
build/block-library/blocks/audio/theme.css 126 B +16 B (+15%) ⚠️
build/block-library/blocks/avatar/style-rtl.css 84 B +25 B (+42%) 🚨
build/block-library/blocks/avatar/style.css 84 B +25 B (+42%) 🚨
build/block-library/blocks/categories/style-rtl.css 100 B +21 B (+27%) 🚨
build/block-library/blocks/categories/style.css 100 B +21 B (+27%) 🚨
build/block-library/blocks/cover/style-rtl.css 1.57 kB +11 B (+1%)
build/block-library/blocks/embed/theme-rtl.css 126 B +16 B (+15%) ⚠️
build/block-library/blocks/embed/theme.css 126 B +16 B (+15%) ⚠️
build/block-library/blocks/image/theme-rtl.css 126 B +16 B (+15%) ⚠️
build/block-library/blocks/image/theme.css 126 B +16 B (+15%) ⚠️
build/block-library/blocks/navigation/editor-rtl.css 1.99 kB +9 B (0%)
build/block-library/blocks/navigation/editor.css 2 kB +9 B (0%)
build/block-library/blocks/navigation/style-rtl.css 2.15 kB +117 B (+6%) 🔍
build/block-library/blocks/navigation/style.css 2.14 kB +115 B (+6%) 🔍
build/block-library/blocks/site-logo/editor-rtl.css 488 B +7 B (+1%)
build/block-library/blocks/site-logo/editor.css 488 B +7 B (+1%)
build/block-library/blocks/site-logo/style-rtl.css 203 B +11 B (+6%) 🔍
build/block-library/blocks/site-logo/style.css 203 B +11 B (+6%) 🔍
build/block-library/blocks/social-links/style-rtl.css 1.4 kB +9 B (+1%)
build/block-library/blocks/social-links/style.css 1.39 kB +9 B (+1%)
build/block-library/blocks/table/theme-rtl.css 190 B +15 B (+9%) 🔍
build/block-library/blocks/table/theme.css 190 B +15 B (+9%) 🔍
build/block-library/blocks/video/theme-rtl.css 126 B +16 B (+15%) ⚠️
build/block-library/blocks/video/theme.css 126 B +16 B (+15%) ⚠️
build/block-library/common-rtl.css 1.02 kB +12 B (+1%)
build/block-library/common.css 1.02 kB +12 B (+1%)
build/block-library/editor-rtl.css 11 kB +13 B (0%)
build/block-library/editor.css 11 kB +13 B (0%)
build/block-library/index.min.js 188 kB -57 B (0%)
build/block-library/style-rtl.css 12 kB +123 B (+1%)
build/block-library/style.css 12 kB +118 B (+1%)
build/block-library/theme-rtl.css 719 B +24 B (+3%)
build/block-library/theme.css 722 B +22 B (+3%)
build/blocks/index.min.js 49.6 kB +12 B (0%)
build/components/index.min.js 198 kB +335 B (0%)
build/components/style-rtl.css 11.5 kB -35 B (0%)
build/components/style.css 11.5 kB -36 B (0%)
build/compose/index.min.js 12 kB +10 B (0%)
build/data/index.min.js 8.06 kB -3 B (0%)
build/date/index.min.js 32.1 kB +68 B (0%)
build/dom/index.min.js 4.7 kB +7 B (0%)
build/edit-navigation/style-rtl.css 3.99 kB +7 B (0%)
build/edit-navigation/style.css 4 kB +6 B (0%)
build/edit-post/index.min.js 30.7 kB +117 B (0%)
build/edit-post/style-rtl.css 6.94 kB +6 B (0%)
build/edit-post/style.css 6.94 kB +6 B (0%)
build/edit-site/index.min.js 58.3 kB +409 B (+1%)
build/edit-site/style-rtl.css 8.23 kB +37 B (0%)
build/edit-site/style.css 8.22 kB +36 B (0%)
build/edit-widgets/index.min.js 16.5 kB +15 B (0%)
build/edit-widgets/style-rtl.css 4.34 kB +7 B (0%)
build/edit-widgets/style.css 4.34 kB +6 B (0%)
build/editor/index.min.js 41.6 kB +36 B (0%)
build/format-library/index.min.js 6.75 kB -3 B (0%)
build/nux/index.min.js 2.05 kB -3 B (0%)
build/redux-routine/index.min.js 2.74 kB +1 B (0%)
build/viewport/index.min.js 1.09 kB +11 B (+1%)
build/widgets/index.min.js 7.19 kB -7 B (0%)
ℹ️ View Unchanged
Filename Size
build/a11y/index.min.js 982 B
build/annotations/index.min.js 2.76 kB
build/api-fetch/index.min.js 2.26 kB
build/autop/index.min.js 2.14 kB
build/blob/index.min.js 475 B
build/block-directory/index.min.js 7.05 kB
build/block-directory/style-rtl.css 990 B
build/block-directory/style.css 991 B
build/block-editor/default-editor-styles-rtl.css 378 B
build/block-editor/default-editor-styles.css 378 B
build/block-library/blocks/archives/editor-rtl.css 61 B
build/block-library/blocks/archives/editor.css 60 B
build/block-library/blocks/archives/style-rtl.css 65 B
build/block-library/blocks/archives/style.css 65 B
build/block-library/blocks/audio/editor-rtl.css 150 B
build/block-library/blocks/audio/editor.css 150 B
build/block-library/blocks/audio/style-rtl.css 122 B
build/block-library/blocks/audio/style.css 122 B
build/block-library/blocks/avatar/editor-rtl.css 116 B
build/block-library/blocks/avatar/editor.css 116 B
build/block-library/blocks/block/editor-rtl.css 161 B
build/block-library/blocks/block/editor.css 161 B
build/block-library/blocks/button/editor-rtl.css 441 B
build/block-library/blocks/button/editor.css 441 B
build/block-library/blocks/button/style-rtl.css 505 B
build/block-library/blocks/button/style.css 505 B
build/block-library/blocks/buttons/editor-rtl.css 292 B
build/block-library/blocks/buttons/editor.css 292 B
build/block-library/blocks/buttons/style-rtl.css 275 B
build/block-library/blocks/buttons/style.css 275 B
build/block-library/blocks/calendar/style-rtl.css 207 B
build/block-library/blocks/calendar/style.css 207 B
build/block-library/blocks/categories/editor-rtl.css 84 B
build/block-library/blocks/categories/editor.css 83 B
build/block-library/blocks/code/editor-rtl.css 53 B
build/block-library/blocks/code/editor.css 53 B
build/block-library/blocks/code/style-rtl.css 103 B
build/block-library/blocks/code/style.css 103 B
build/block-library/blocks/code/theme-rtl.css 124 B
build/block-library/blocks/code/theme.css 124 B
build/block-library/blocks/columns/editor-rtl.css 108 B
build/block-library/blocks/columns/editor.css 108 B
build/block-library/blocks/columns/style-rtl.css 406 B
build/block-library/blocks/columns/style.css 406 B
build/block-library/blocks/comment-author-avatar/editor-rtl.css 125 B
build/block-library/blocks/comment-author-avatar/editor.css 125 B
build/block-library/blocks/comment-content/style-rtl.css 92 B
build/block-library/blocks/comment-content/style.css 92 B
build/block-library/blocks/comment-template/style-rtl.css 187 B
build/block-library/blocks/comment-template/style.css 185 B
build/block-library/blocks/comments-pagination-numbers/editor-rtl.css 123 B
build/block-library/blocks/comments-pagination-numbers/editor.css 121 B
build/block-library/blocks/comments-pagination/editor-rtl.css 222 B
build/block-library/blocks/comments-pagination/editor.css 209 B
build/block-library/blocks/comments-pagination/style-rtl.css 235 B
build/block-library/blocks/comments-pagination/style.css 231 B
build/block-library/blocks/comments-title/editor-rtl.css 75 B
build/block-library/blocks/comments-title/editor.css 75 B
build/block-library/blocks/comments/editor-rtl.css 834 B
build/block-library/blocks/comments/editor.css 832 B
build/block-library/blocks/comments/style-rtl.css 632 B
build/block-library/blocks/comments/style.css 630 B
build/block-library/blocks/cover/editor-rtl.css 605 B
build/block-library/blocks/cover/editor.css 607 B
build/block-library/blocks/cover/style.css 1.55 kB
build/block-library/blocks/embed/editor-rtl.css 293 B
build/block-library/blocks/embed/editor.css 293 B
build/block-library/blocks/embed/style-rtl.css 410 B
build/block-library/blocks/embed/style.css 410 B
build/block-library/blocks/file/editor-rtl.css 300 B
build/block-library/blocks/file/editor.css 300 B
build/block-library/blocks/file/style-rtl.css 253 B
build/block-library/blocks/file/style.css 254 B
build/block-library/blocks/file/view.min.js 346 B
build/block-library/blocks/freeform/editor-rtl.css 2.44 kB
build/block-library/blocks/freeform/editor.css 2.44 kB
build/block-library/blocks/gallery/editor-rtl.css 948 B
build/block-library/blocks/gallery/editor.css 950 B
build/block-library/blocks/gallery/style-rtl.css 1.53 kB
build/block-library/blocks/gallery/style.css 1.53 kB
build/block-library/blocks/gallery/theme-rtl.css 108 B
build/block-library/blocks/gallery/theme.css 108 B
build/block-library/blocks/group/editor-rtl.css 337 B
build/block-library/blocks/group/editor.css 337 B
build/block-library/blocks/group/style-rtl.css 57 B
build/block-library/blocks/group/style.css 57 B
build/block-library/blocks/group/theme-rtl.css 78 B
build/block-library/blocks/group/theme.css 78 B
build/block-library/blocks/heading/style-rtl.css 76 B
build/block-library/blocks/heading/style.css 76 B
build/block-library/blocks/html/editor-rtl.css 327 B
build/block-library/blocks/html/editor.css 329 B
build/block-library/blocks/image/editor-rtl.css 876 B
build/block-library/blocks/image/editor.css 873 B
build/block-library/blocks/image/style-rtl.css 627 B
build/block-library/blocks/image/style.css 630 B
build/block-library/blocks/latest-comments/style-rtl.css 284 B
build/block-library/blocks/latest-comments/style.css 284 B
build/block-library/blocks/latest-posts/editor-rtl.css 213 B
build/block-library/blocks/latest-posts/editor.css 212 B
build/block-library/blocks/latest-posts/style-rtl.css 463 B
build/block-library/blocks/latest-posts/style.css 462 B
build/block-library/blocks/list/style-rtl.css 88 B
build/block-library/blocks/list/style.css 88 B
build/block-library/blocks/media-text/editor-rtl.css 266 B
build/block-library/blocks/media-text/editor.css 263 B
build/block-library/blocks/media-text/style-rtl.css 507 B
build/block-library/blocks/media-text/style.css 505 B
build/block-library/blocks/more/editor-rtl.css 431 B
build/block-library/blocks/more/editor.css 431 B
build/block-library/blocks/navigation-link/editor-rtl.css 705 B
build/block-library/blocks/navigation-link/editor.css 703 B
build/block-library/blocks/navigation-link/style-rtl.css 115 B
build/block-library/blocks/navigation-link/style.css 115 B
build/block-library/blocks/navigation-submenu/editor-rtl.css 296 B
build/block-library/blocks/navigation-submenu/editor.css 295 B
build/block-library/blocks/navigation-submenu/view.min.js 423 B
build/block-library/blocks/navigation/view-modal.min.js 2.78 kB
build/block-library/blocks/navigation/view.min.js 443 B
build/block-library/blocks/nextpage/editor-rtl.css 395 B
build/block-library/blocks/nextpage/editor.css 395 B
build/block-library/blocks/page-list/editor-rtl.css 363 B
build/block-library/blocks/page-list/editor.css 363 B
build/block-library/blocks/page-list/style-rtl.css 175 B
build/block-library/blocks/page-list/style.css 175 B
build/block-library/blocks/paragraph/editor-rtl.css 174 B
build/block-library/blocks/paragraph/editor.css 174 B
build/block-library/blocks/paragraph/style-rtl.css 260 B
build/block-library/blocks/paragraph/style.css 260 B
build/block-library/blocks/post-author/style-rtl.css 175 B
build/block-library/blocks/post-author/style.css 176 B
build/block-library/blocks/post-comments-form/editor-rtl.css 96 B
build/block-library/blocks/post-comments-form/editor.css 96 B
build/block-library/blocks/post-comments-form/style-rtl.css 493 B
build/block-library/blocks/post-comments-form/style.css 493 B
build/block-library/blocks/post-date/style-rtl.css 61 B
build/block-library/blocks/post-date/style.css 61 B
build/block-library/blocks/post-excerpt/editor-rtl.css 73 B
build/block-library/blocks/post-excerpt/editor.css 73 B
build/block-library/blocks/post-excerpt/style-rtl.css 69 B
build/block-library/blocks/post-excerpt/style.css 69 B
build/block-library/blocks/post-featured-image/editor-rtl.css 507 B
build/block-library/blocks/post-featured-image/editor.css 505 B
build/block-library/blocks/post-featured-image/style-rtl.css 166 B
build/block-library/blocks/post-featured-image/style.css 166 B
build/block-library/blocks/post-template/editor-rtl.css 99 B
build/block-library/blocks/post-template/editor.css 98 B
build/block-library/blocks/post-template/style-rtl.css 282 B
build/block-library/blocks/post-template/style.css 282 B
build/block-library/blocks/post-terms/style-rtl.css 73 B
build/block-library/blocks/post-terms/style.css 73 B
build/block-library/blocks/post-title/style-rtl.css 100 B
build/block-library/blocks/post-title/style.css 100 B
build/block-library/blocks/preformatted/style-rtl.css 103 B
build/block-library/blocks/preformatted/style.css 103 B
build/block-library/blocks/pullquote/editor-rtl.css 135 B
build/block-library/blocks/pullquote/editor.css 135 B
build/block-library/blocks/pullquote/style-rtl.css 326 B
build/block-library/blocks/pullquote/style.css 325 B
build/block-library/blocks/pullquote/theme-rtl.css 167 B
build/block-library/blocks/pullquote/theme.css 167 B
build/block-library/blocks/query-pagination-numbers/editor-rtl.css 122 B
build/block-library/blocks/query-pagination-numbers/editor.css 121 B
build/block-library/blocks/query-pagination/editor-rtl.css 221 B
build/block-library/blocks/query-pagination/editor.css 211 B
build/block-library/blocks/query-pagination/style-rtl.css 282 B
build/block-library/blocks/query-pagination/style.css 278 B
build/block-library/blocks/query-title/style-rtl.css 63 B
build/block-library/blocks/query-title/style.css 63 B
build/block-library/blocks/query/editor-rtl.css 439 B
build/block-library/blocks/query/editor.css 439 B
build/block-library/blocks/quote/style-rtl.css 213 B
build/block-library/blocks/quote/style.css 213 B
build/block-library/blocks/quote/theme-rtl.css 223 B
build/block-library/blocks/quote/theme.css 226 B
build/block-library/blocks/read-more/style-rtl.css 132 B
build/block-library/blocks/read-more/style.css 132 B
build/block-library/blocks/rss/editor-rtl.css 202 B
build/block-library/blocks/rss/editor.css 204 B
build/block-library/blocks/rss/style-rtl.css 289 B
build/block-library/blocks/rss/style.css 288 B
build/block-library/blocks/search/editor-rtl.css 165 B
build/block-library/blocks/search/editor.css 165 B
build/block-library/blocks/search/style-rtl.css 396 B
build/block-library/blocks/search/style.css 393 B
build/block-library/blocks/search/theme-rtl.css 114 B
build/block-library/blocks/search/theme.css 114 B
build/block-library/blocks/separator/editor-rtl.css 146 B
build/block-library/blocks/separator/editor.css 146 B
build/block-library/blocks/separator/style-rtl.css 233 B
build/block-library/blocks/separator/style.css 233 B
build/block-library/blocks/separator/theme-rtl.css 194 B
build/block-library/blocks/separator/theme.css 194 B
build/block-library/blocks/shortcode/editor-rtl.css 464 B
build/block-library/blocks/shortcode/editor.css 464 B
build/block-library/blocks/site-tagline/editor-rtl.css 86 B
build/block-library/blocks/site-tagline/editor.css 86 B
build/block-library/blocks/site-title/editor-rtl.css 84 B
build/block-library/blocks/site-title/editor.css 84 B
build/block-library/blocks/social-link/editor-rtl.css 184 B
build/block-library/blocks/social-link/editor.css 184 B
build/block-library/blocks/social-links/editor-rtl.css 674 B
build/block-library/blocks/social-links/editor.css 673 B
build/block-library/blocks/spacer/editor-rtl.css 322 B
build/block-library/blocks/spacer/editor.css 322 B
build/block-library/blocks/spacer/style-rtl.css 48 B
build/block-library/blocks/spacer/style.css 48 B
build/block-library/blocks/table/editor-rtl.css 494 B
build/block-library/blocks/table/editor.css 494 B
build/block-library/blocks/table/style-rtl.css 611 B
build/block-library/blocks/table/style.css 609 B
build/block-library/blocks/tag-cloud/style-rtl.css 239 B
build/block-library/blocks/tag-cloud/style.css 239 B
build/block-library/blocks/template-part/editor-rtl.css 235 B
build/block-library/blocks/template-part/editor.css 235 B
build/block-library/blocks/template-part/theme-rtl.css 101 B
build/block-library/blocks/template-part/theme.css 101 B
build/block-library/blocks/text-columns/editor-rtl.css 95 B
build/block-library/blocks/text-columns/editor.css 95 B
build/block-library/blocks/text-columns/style-rtl.css 166 B
build/block-library/blocks/text-columns/style.css 166 B
build/block-library/blocks/verse/style-rtl.css 87 B
build/block-library/blocks/verse/style.css 87 B
build/block-library/blocks/video/editor-rtl.css 561 B
build/block-library/blocks/video/editor.css 563 B
build/block-library/blocks/video/style-rtl.css 174 B
build/block-library/blocks/video/style.css 174 B
build/block-library/editor-elements-rtl.css 75 B
build/block-library/editor-elements.css 75 B
build/block-library/elements-rtl.css 54 B
build/block-library/elements.css 54 B
build/block-library/reset-rtl.css 478 B
build/block-library/reset.css 478 B
build/block-serialization-default-parser/index.min.js 1.1 kB
build/block-serialization-spec-parser/index.min.js 2.83 kB
build/core-data/index.min.js 15.5 kB
build/customize-widgets/index.min.js 11.3 kB
build/customize-widgets/style-rtl.css 1.38 kB
build/customize-widgets/style.css 1.38 kB
build/data-controls/index.min.js 653 B
build/deprecated/index.min.js 507 B
build/dom-ready/index.min.js 324 B
build/edit-navigation/index.min.js 16 kB
build/edit-post/classic-rtl.css 546 B
build/edit-post/classic.css 547 B
build/editor/style-rtl.css 3.66 kB
build/editor/style.css 3.65 kB
build/element/index.min.js 4.68 kB
build/escape-html/index.min.js 537 B
build/format-library/style-rtl.css 571 B
build/format-library/style.css 571 B
build/hooks/index.min.js 1.64 kB
build/html-entities/index.min.js 448 B
build/i18n/index.min.js 3.77 kB
build/is-shallow-equal/index.min.js 527 B
build/keyboard-shortcuts/index.min.js 1.78 kB
build/keycodes/index.min.js 1.81 kB
build/list-reusable-blocks/index.min.js 1.74 kB
build/list-reusable-blocks/style-rtl.css 835 B
build/list-reusable-blocks/style.css 835 B
build/media-utils/index.min.js 2.93 kB
build/notices/index.min.js 953 B
build/nux/style-rtl.css 732 B
build/nux/style.css 728 B
build/plugins/index.min.js 1.94 kB
build/preferences-persistence/index.min.js 2.22 kB
build/preferences/index.min.js 1.3 kB
build/primitives/index.min.js 933 B
build/priority-queue/index.min.js 612 B
build/react-i18n/index.min.js 696 B
build/react-refresh-entry/index.min.js 8.44 kB
build/react-refresh-runtime/index.min.js 7.31 kB
build/reusable-blocks/index.min.js 2.21 kB
build/reusable-blocks/style-rtl.css 256 B
build/reusable-blocks/style.css 256 B
build/rich-text/index.min.js 10.4 kB
build/server-side-render/index.min.js 1.61 kB
build/shortcode/index.min.js 1.53 kB
build/token-list/index.min.js 644 B
build/url/index.min.js 3.61 kB
build/vendors/react-dom.min.js 38.5 kB
build/vendors/react.min.js 4.34 kB
build/warning/index.min.js 268 B
build/widgets/style-rtl.css 1.18 kB
build/widgets/style.css 1.19 kB
build/wordcount/index.min.js 1.06 kB

compressed-size-action

// When step is "any" clamp the value, otherwise round and clamp it.
return isStepAny
? Math.min( max, Math.max( min, value ) )
: roundClamp( value, min, max, stepOverride ?? baseStep );
};

const autoComplete = typeProp === 'number' ? 'off' : null;
const autoComplete = typeProp === 'number' ? 'off' : undefined;
Copy link
Member Author

Choose a reason for hiding this comment

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

⚠️ Runtime change: null is an invalid value for the autoComplete attribute.

packages/components/src/number-control/index.tsx Outdated Show resolved Hide resolved
@@ -259,7 +259,6 @@ function UnforwardedUnitControl(
return (
<Root className="components-unit-control-wrapper" style={ style }>
<ValueInput
aria-label={ label }
Copy link
Member Author

Choose a reason for hiding this comment

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

⚠️ Runtime change: This label variable is a ReactNode, but aria-label only accepts strings. I went ahead and removed this redundant aria-label, because the same value is added on line 270 via label prop.

Copy link
Contributor

Choose a reason for hiding this comment

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

Noted. Basically the input will be labelled via an actual label element (with the for attribute), instead of the aria-label attribute 👌

packages/components/src/unit-control/types.ts Show resolved Hide resolved
@ciampo
Copy link
Contributor

ciampo commented Sep 2, 2022

Going to leave a review at later / early next week, but in the meantime I'm going to leave a link to my previous attempt #38753 as it may be useful

Copy link
Contributor

@ciampo ciampo left a comment

Choose a reason for hiding this comment

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

Good job! I left a few comments, a good chunk of which are inspired by the previous work in #38753.

Regarding the migration to TypeScript, the main part that still need to be addressed is which type we should assign to the value prop. I wrote a good recap of the situation in #40535 (comment). Although I would understand if we preferred to defer that decision to a separate PR, for the sake of getting this PR merged quickly and unblock further work that depends on NumberControl being typed

packages/components/src/number-control/types.ts Outdated Show resolved Hide resolved
@@ -259,7 +259,6 @@ function UnforwardedUnitControl(
return (
<Root className="components-unit-control-wrapper" style={ style }>
<ValueInput
aria-label={ label }
Copy link
Contributor

Choose a reason for hiding this comment

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

Noted. Basically the input will be labelled via an actual label element (with the for attribute), instead of the aria-label attribute 👌

packages/components/src/unit-control/types.ts Outdated Show resolved Hide resolved
packages/components/src/number-control/index.tsx Outdated Show resolved Hide resolved
packages/components/src/number-control/types.ts Outdated Show resolved Hide resolved
packages/components/src/number-control/index.tsx Outdated Show resolved Hide resolved
packages/components/src/number-control/index.tsx Outdated Show resolved Hide resolved
packages/components/src/number-control/index.tsx Outdated Show resolved Hide resolved
packages/components/src/number-control/index.tsx Outdated Show resolved Hide resolved
packages/components/src/number-control/index.tsx Outdated Show resolved Hide resolved
@mirka
Copy link
Member Author

mirka commented Sep 6, 2022

Regarding the migration to TypeScript, the main part that still need to be addressed is which type we should assign to the value prop. I wrote a good recap of the situation in #40535 (comment). Although I would understand if we preferred to defer that decision to a separate PR, for the sake of getting this PR merged quickly and unblock further work that depends on NumberControl being typed

Yes exactly, the main motivation of this PR is to unblock our way to clearing out the excludes in tsconfig, and to get Storybook docs to an acceptable state. I don't intend to solve the value puzzle at this time, sorry 😅

@mirka mirka requested a review from ciampo September 6, 2022 17:04
@@ -138,7 +138,8 @@ function UnforwardedRangeControl< IconProps = unknown >(
onChange( nextValue );
};

const handleOnChange = ( next: string ) => {
const handleOnChange = ( next?: string ) => {
// @ts-expect-error TODO: Investigate if this is problematic
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this use ensureNumber ?

Copy link
Member Author

Choose a reason for hiding this comment

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

This one can't be fixed by ensureNumber because the problem is that next can potentially be undefined.

Technically, we can remove the ts-ignore without changing effective runtime behavior by doing something like this:

let nextValue = typeof next === 'undefined' ? NaN : parseFloat( next );

But then again, I really think this one needs further investigation because it's unclear whether it's intended for setValue() to sometimes receive a NaN. It could actually be problematic, so I wouldn't want to imply that this behavior is legit.

const applyEmptyValue =
required === false && currentValue === '';

// @ts-expect-error TODO: Investigate if this is wrong
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of "Investigate if this is wrong", should the comment be more about the fact that this error will only be solved once we make a decision on the value type?

(same for the rest of the @ts-expect-error comments about the type of the value prop)

Copy link
Member Author

@mirka mirka Sep 7, 2022

Choose a reason for hiding this comment

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

Good call, I improved the TODO comments all around. 90948f0

One of the interesting causes was the isValueEmpty() util function. I was able to fix the typing, but it's a bit out of scope for this PR so I'll submit it separately.

packages/components/src/number-control/index.tsx Outdated Show resolved Hide resolved
packages/components/src/number-control/index.tsx Outdated Show resolved Hide resolved
@mirka mirka requested a review from ciampo September 7, 2022 12:18
Copy link
Contributor

@ciampo ciampo left a comment

Choose a reason for hiding this comment

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

🚀

Good to be merged once we add a CHANGELOG entry

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Package] Components /packages/components [Type] Code Quality Issues or PRs that relate to code quality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants