ResponsiveWrapper: Convert to TypeScript #46480
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: +27 B (0%) Total Size: 1.32 MB
ℹ️ View Unchanged
|
mirka
commented
Dec 12, 2022
| const [ containerResizeListener, { width: containerWidth } ] = | ||
| useResizeObserver(); | ||
| if ( Children.count( children ) !== 1 ) { | ||
| return null; | ||
| } | ||
| const imageStyle = { | ||
| paddingBottom: | ||
| naturalWidth < containerWidth | ||
| naturalWidth < ( containerWidth ?? 0 ) |
There was a problem hiding this comment.
TS was not happy because containerWidth can potentially be null. (Apparently useResizeObserver() will return null for this value until after the first render.)
My code change here just makes explicit what was happening implicitly — a null will be interpreted as 0 when compared with the < operator. This maintains the current behavior while assuring TS that it's ok.
There was a problem hiding this comment.
Excellent explanation, saved a bunch of time on my end and will make it clear for anyone looking into the reasons behind this code change. Thank you!
ciampo
approved these changes
Jan 6, 2023
| const [ containerResizeListener, { width: containerWidth } ] = | ||
| useResizeObserver(); | ||
| if ( Children.count( children ) !== 1 ) { | ||
| return null; | ||
| } | ||
| const imageStyle = { | ||
| paddingBottom: | ||
| naturalWidth < containerWidth | ||
| naturalWidth < ( containerWidth ?? 0 ) |
There was a problem hiding this comment.
Excellent explanation, saved a bunch of time on my end and will make it clear for anyone looking into the reasons behind this code change. Thank you!
| /** | ||
| * The element to wrap. | ||
| */ | ||
| children: React.ReactElement; |
There was a problem hiding this comment.
The choice of ReactElement seems correct. We can also broaden the type if necessary.
| */ | ||
| import ResponsiveWrapper from '..'; | ||
|
|
||
| const meta: ComponentMeta< typeof ResponsiveWrapper > = { |
There was a problem hiding this comment.
Should we hide children's control? It currently shows a react object, which is not really user-friendly to edit
ciampo
reviewed
Jan 6, 2023
packages/components/CHANGELOG.md
Outdated
| @@ -43,6 +43,7 @@ | |||
| - `useBaseField`: Convert to TypeScript ([#45712](https://github.com/WordPress/gutenberg/pull/45712)). | |||
| - `Dashicon`: Convert to TypeScript ([#45924](https://github.com/WordPress/gutenberg/pull/45924)). | |||
| - `PaletteEdit`: add follow up changelog for #45681 and tests [#46095](https://github.com/WordPress/gutenberg/pull/46095). | |||
| - `ResponsiveWrapper`: Convert to TypeScript ([#46480](https://github.com/WordPress/gutenberg/pull/46480)). | |||
There was a problem hiding this comment.
May need repositioning after the last package release
|
Flaky tests detected in a70bf32. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/3858579356
|
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?
Add TypeScript types and documentation to the
ResponsiveWrappercomponent.Why?
So we can ship TS data with the wp-components package.
Testing Instructions
npm run storybook:devResponsiveWrappershould look good, and the prop descriptions should match the README.md.