Flex: Convert component to TypeScript #42537
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
walbo
added
[Type] Enhancement
|
Size Change: +2 B (0%) Total Size: 1.26 MB
ℹ️ View Unchanged
|
mirka
approved these changes
Jul 22, 2022
Comment on lines
+37
to
+39
| const GrayBox = ( { children }: { children: string } ) => ( | ||
| <View style={ { backgroundColor: '#eee', padding: 10 } }>{ children }</View> | ||
| ); |
There was a problem hiding this comment.
This styling makes it a lot more understandable 💛
| }; | ||
| Default.args = {}; | ||
|
|
||
| export const ResponsiveDirection: ComponentStory< typeof Flex > = ( { |
There was a problem hiding this comment.
I can't decide between "add an explanation of the useResponsiveValue system, which we might not want to encourage" or "remove this story and risk everyone forgetting that the feature exists". So maybe for now we go with the path of least resistance: Keep the story but without a description 😂
There was a problem hiding this comment.
Agreed. I would at most add a comment discouraging usage of useResponsiveValue system as it may be soon deprecated (or do nothing as you suggested)
There was a problem hiding this comment.
Yep. Lets keep it as it is 👍
ciampo
reviewed
Jul 24, 2022
ciampo
reviewed
Jul 24, 2022
ciampo
reviewed
Jul 24, 2022
|
|
||
| **Type**: `[number | string]` | ||
| ### `gap`: `number` |
There was a problem hiding this comment.
This is another change that we may consider: I personally find it confusing that we exposed align and justify behaving exactly like the CSS properties, while we use an internal grid system for gap (which is also a valid CSS prop for flex layouts)
@mirka what are your thoughts on this one?
There was a problem hiding this comment.
This is a problem with SpaceInput values in general, but I agree that the polymorphic nature of those values can be confusing. (Especially since React style props interpret unitless number values to be pixels by default!) Unfortunately I can't think of an easy way around it, other than removing SpaceInput support from the components themselves and make the consumer use space() from the outside, if they need to.
As an aside, I believe the internal gap-to-margin implementation we have right now was kind of meant as a polyfill, since Safari didn't support flex gap when the Flex component was first written. I don't know if there are active bugs in there, but we can look into removing it in a separate issue. It would be nice to simply rely on native CSS gap.
There was a problem hiding this comment.
This is a problem with SpaceInput values in general, but I agree that the polymorphic nature of those values can be confusing. (Especially since React style props interpret unitless number values to be pixels by default!) Unfortunately I can't think of an easy way around it, other than removing SpaceInput support from the components themselves and make the consumer use space() from the outside, if they need to.
Yeah, this may require some more investigation
As an aside, I believe the internal gap-to-margin implementation we have right now was kind of meant as a polyfill, since Safari didn't support flex gap when the Flex component was first written. I don't know if there are active bugs in there, but we can look into removing it in a separate issue. It would be nice to simply rely on native CSS gap.
I get the same feeling — it would be definitely great to rely on native CSS though
There was a problem hiding this comment.
Yes. I would be great exploring if the native gap can be used. I guess its out of scope for this PR tough.
Co-authored-by: Lena Morita <lena@jaguchi.com>
Co-authored-by: Lena Morita <lena@jaguchi.com>
|
Anything feedback I missed or is this PR ready to 🚢 ? |
🚢 |
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.
What?
Converts the
Flex,FlexItemandFlexBlockcomponents to TypeScript.Why?
Part of the @wordpress/components's TypeScript migration (#35744).
How?
Flex,FlexItemandFlexBlockto TypeScript.Testing Instructions
Flexcontinues to function as expected