Flex component - add 'direction' prop. #30261
Closed
Conversation
This file contains hidden or 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
Addison-Stavlo
requested review from
ajitbohra and
chrisvanpatten
as code owners
Addison-Stavlo
requested review from
Copons,
creativecoder,
david-szabo97,
jeyip,
mattwiebe and
vindl
Addison-Stavlo
commented
Mar 26, 2021
Comment on lines
65
to
66
| // @ts-ignore | ||
| direction={ simplifiedDirection } |
Contributor
Author
There was a problem hiding this comment.
TS was angry about this potentially being undefined when we have noted it as a string. But if we look at the code above, it would always be a string. So @ts-ignore for now unless there are better suggestions.
Member
There was a problem hiding this comment.
simplifiedDirection is a string beucase of direction.slice( 0, -8). That's why TS is raging. I'm not sure if it is possible to force a type on a specific variable with JSDoc. simplifiedDirection as FlexDirection could fix the problem in TS.
I think you are better off with an explicit function types-wise:
/** @typedef {import('react').CSSProperties['flexDirection'] | 'row' | 'column'} FlexDirection */
^^^ put this into `/* eslint-disable jsdoc/valid-types */` above OwnProps
* @property {FlexDirection} direction Sets the flex-direction property.
^^^
and replace the type in OwnProps
/**
*
* @param {FlexDirection} direction
* @return {FlexDirection} direction
*/
function simplifyDirection( direction ) {
if ( direction === 'column-reverse' ) {
return 'column';
}
if ( direction === 'row-reverse' ) {
return 'row';
}
return direction;
}then you can use it like
const simplifiedDirection = simplifyDirection( direction );and you can remove the @ts-ignore
|
Size Change: +4.68 kB (0%) Total Size: 1.42 MB
ℹ️ View Unchanged
|
Contributor
I'm not quite a fan of this. 🤔 If we want to support flex-direction's Otherwise, we might as well keep |
7 tasks
Contributor
|
Given the conflict with |
Contributor
Author
I think this makes sense, to clarify: So would that mean use |
Contributor
|
I was thinking something like this: function Flex( { direction, isReversed } ) {
const directionArray = direction.split( '-' );
let actualDirection = direction[0];
if ( isReversed && directionArray.length === 0 ) {
actualDirection += '-reverse';
}
return <div style={ { flexDirection: actualDirection } } />;
} |
Contributor
|
Sorry for closing, hit the wrong button! |
Contributor
Author
Im confused, in that example if |
Contributor
Author
|
Going off @sarayourfriend's comment about deprecating direction overrides isReversed: or or something else? |
Contributor
@Addison-Stavlo let's ignore my error of not considering the difference between Currently the component only support For this reason, I'm inclined to either ignore Don't have a strong opinion either way. I would conceptually like that |
Contributor
Author
Yeah, that might make the most sense. 🤔 |
Contributor
|
To me deprecation means this: function useDirection({ isReversed, direction }) {
if ( typeof isReversed !== 'undefined' ) {
deprecated( 'Flex isReversed', { alternative: 'Flex direction="row-reverse"' } );
if ( isReversed ) {
return 'row-reverse';
}
return 'row';
}
return direction;
}And then the component just does: function Flex( props ) {
const direction = useDirection( props ):
const { otherProp } = props;
// ...That way the component can ignore |
Contributor
Author
|
Thanks @sarayourfriend! That clarifies the idea a bit for sure. 😁 This seems to be working as expected with 117dcc7 .
|
Member
|
Works well for me! How about adding some stories? |
5 tasks
Member
|
Duh, looks like #31297 superseded this PR. |
Contributor
Author
|
Yeah, I'm happy with that 😁 . Now that it is merged il close this out. |
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.
Description
I noticed this neat
Flexcomponent and went to use it in a vertical/column direction. It seems that currently the vertical orientation (flex-direction: column) is not supported. This PR attempts to add this functionality.This is slightly odd since the currently existing
isReversedfunction sets theflex-directioncss property by assuming row. Lets consider deprecatingisReversedin favor ofdirection. If theisReversedprop is present, a deprecation warning is thrown and the component has the same behavior as it previously did. Otherwise, we go with the newdirectionprop defaulting to 'row'.How has this been tested?
Test the component with the deprecated
isReversedprop and ensure their are no regressions.Test the component with the new
directionprop and ensure settings work as expected.Screenshots
Types of changes
Checklist: