Skip to content
This repository was archived by the owner on Mar 4, 2020. It is now read-only.

fix(Flex.Item): shrink issue #1086

Merged
kuzhelov merged 3 commits into from
Mar 21, 2019
Merged

fix(Flex.Item): shrink issue #1086

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
47d0176
fix shrink issue
kuzhelov-ms Mar 21, 2019
d2d62e7
upd changelog
kuzhelov-ms Mar 21, 2019
54abdcf
Merge branch 'master' into fix/flex-item-shrink
kuzhelov Mar 21, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm
### Fixes
- Add aria posinset and setsize, hide menu indicator from narration @jurokapsiar ([#1066](https://github.com/stardust-ui/react/pull/1066))
- Fix applying accessibility key handlers @layershifter ([#1072](https://github.com/stardust-ui/react/pull/1072))
- Fix `shrink` prop behavior for `Flex.Item` @kuzhelov ([#1086](https://github.com/stardust-ui/react/pull/1086))

### Features
- Add `Alert` component @Bugaa92 ([#1063](https://github.com/stardust-ui/react/pull/1063))
Expand Down
2 changes: 1 addition & 1 deletion packages/react/src/themes/teams/components/Flex/flexItemStyles.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ const flexItemStyles: ComponentSlotStylesInput<FlexItemProps, FlexItemVariables>

...(p.size && toFlexItemSizeValues(v[p.size])),

...(p.shrink && { flexShrink: p.shrink }),
...(typeof p.shrink === 'number' && { flexShrink: p.shrink }),
Copy link
Collaborator

@bmdalex bmdalex Mar 21, 2019

Choose a reason for hiding this comment

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

I know this is already merged, sorry for not being able to review before that. However, there are a few things I'd like to mention.

1. what about the global values as string?

I think we should allow users to pass these too (see: https://developer.mozilla.org/en-US/docs/Web/CSS/flex-shrink#Syntax)
image

2. what about the true boolean value?

From the new code it looks like

<Flex.Item shrink>
  {/* some element */}
</Flex.Item />

will have no effect; to me that's not expected user behavior since the type of shrink prop is still boolean | number

Copy link
Contributor Author

@kuzhelov kuzhelov Mar 21, 2019

Choose a reason for hiding this comment

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

Let me start with the second question first - yes, this is exactly the effect you've described. The logic is correct as<Flex.Item shrink /> is, essentially, equivalent to <Flex.Item />, given that flex items are shrinkable by default (we've done it to correspond to CSS specs).

Speaking of the first question - we might want to introduce support for mentioned props later, but for now extra prop values from the scope of Flex component were omitted in order to achieve the following:

address as many scenarios with the least set of props/values necessary

Thus, might suggest to leave it as is for now and return to the question of introducing these prop values (or use another solution approaches) when there will be an appealing use case for that.

...(p.shrink === false && { flexShrink: 0 }),

...(p.grow && { flexGrow: p.grow }),
Expand Down