[OSCI] Adding props for shrink and basis for OuiFlexItem #1126

BigSamu · 2023-10-30T13:31:06Z

Description

Props shrink and basis have been added for OuiFlexItem. Behaviour similar to grow prop. Accepted values:

shrink: true, false, null, 1..10
basis: true, false, null, 'auto', 'max-content', 'min-content', 'fit-content',

Issues Resolved

Fixes issue #776

Check List

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

For more information on following Developer Certificate of Origin and signing off your commits, please check here.

Signed-off-by: Samuel Valdes Gutierrez <[email protected]>

…ts tests for testing Signed-off-by: Samuel Valdes Gutierrez <[email protected]>

Signed-off-by: Samuel Valdes Gutierrez <[email protected]>

… place Signed-off-by: Samuel Valdes Gutierrez <[email protected]>

Signed-off-by: Samuel Valdes Gutierrez <[email protected]>

…flex_item.tsx Signed-off-by: Samuel Valdes Gutierrez <[email protected]>

Signed-off-by: Samuel Valdes Gutierrez <[email protected]>

BigSamu · 2023-11-01T23:09:06Z

@BSFishy @joshuarrrr,

Final updates in PR are ready, with documentation added for props shrink and basis. I would appreciate your feedback on the entire PR.

Just one thing from my side. I think it is a bit cumbersome to consider OuiFlexItem to have the properties flex-grow: 1 and flex-basis: 0%, by default. I feel that we should follow an approach like the one here in W3School, where we can explain the props grow, shrink and basis altogether in one section and also not considering a default behaviour of growing.

Not sure. Maybe the UX team have something to say here.

Regards,

Samuel

BSFishy

Just a few things, looping in @kgcreative for advice on how the docsite should be updated

src/components/flex/flex_item.tsx

BSFishy · 2023-11-02T17:55:27Z

src/components/flex/flex_item.tsx

@@ -61,18 +96,30 @@ export const OuiFlexItem: FunctionComponent<
 > = ({
  children,
  className,
-  grow = true,
+  grow = true, // default true -> flex-grow: 1 and flex-basis: 0%
+  shrink = null, // default null for flex-shrink


The default value for this is 1: https://developer.mozilla.org/en-US/docs/Web/CSS/flex-shrink#formal_definition. Should we make that the default value here?

cc @kgcreative

Hi Matt, Yes, you are right; the default value for the flex-grow CSS property is 1 according to Mozilla docs. However, the way how an OuiFlexItem is defined in OUI considers that these elements always grow inside the flex parent OuiFlexGroup by default (this was the initial design for that element). Thus the prop grow=true, which allows keeping the CSS properties flex-grow: 1 and flex-basis: 0%, both of them coming from _flex_group.scss file.

I'm not sure why in _flex_group.scss we are defining these properties for a OuiFlexItem. The styling for the latter ones should only be allocated in the _flex_item.scss file, in my opinion.

Right, I think we should maintain functionality as much as possible. Since flex-shrink isn't specified anywhere, I think we should make it default to what it would have been previous to this change

(For clarity, this comment is referring to the shrink default value)

So, when you define the default value for shrink as null (i.e.shrink=null), this prop does not apply any CSS property related to flex-shrink. The OuiFlexElement inherits what comes from the parent (OuiFlexGroup) or the default from the Browser (which is flex-shrink:1). So, the default value I use works for this case without changing the design implementation for the whole element.

BSFishy · 2023-11-02T17:55:58Z

src/components/flex/flex_item.tsx

-  grow = true,
+  grow = true, // default true -> flex-grow: 1 and flex-basis: 0%
+  shrink = null, // default null for flex-shrink
+  basis = null, // default null flex-basis


I think the default value for basis should be auto: https://developer.mozilla.org/en-US/docs/Web/CSS/flex-basis#formal_definition

Same as above, If I change the default value of the prop basis to auto, then you will change the default behaviour of grow=true, which means flex-grow:1 and flex-basis: 0%. I agree with what you suggest, but this change will need a whole new design of an OuiFlexItem in terms that they should not grow by default inside an OuiFlexGroup element. Make sense?

Right, that makes sense, so I think we should have some special logic here:

If basis isn't null, use that value

If grow is not falsy, set basis to 0% (add option for 0)

If grow is falsy, set basis to auto

My comments here:

If basis isn't null, use that value It is done when you use one of these options in the prop basis: auto, max-content, min-content, fit-content

If grow is not falsy, set basis to 0% (add option for 0) flex-basis: 0% is already setup by default. For whatever not falsy value this CSS property will kept. Coming from _flex_group.scss

If grow is falsy, set basis to auto Already done when you say grow=false using the class ouiFlexItem--flexGrowZero

CHANGELOG.md

Signed-off-by: Samuel Valdes Gutierrez <[email protected]>

BigSamu · 2023-11-04T13:50:03Z

Hi @BSFishy,

I already replied to your comments for your code review. Looking forward your reply to see how I proceed.

Regards,

Samuel

…pective snapshoots Signed-off-by: Samuel Valdes Gutierrez <[email protected]>

…inkZero Signed-off-by: Samuel Valdes Gutierrez <[email protected]>

BSFishy

The main thing is in one of these comments. Basically, we don't want to apply new styles unless the builder specifies new props.

src/components/flex/flex_item.tsx

…ntainers Signed-off-by: Samuel Valdes Gutierrez <[email protected]>

BigSamu · 2023-11-21T16:23:21Z

@BSFishy,

Updates are ready with the last code review done from your side! Any comments, let me know.

Regards,

Samuel

BSFishy · 2023-12-05T17:43:43Z

Looks great!

BigSamu · 2024-01-01T16:25:18Z

Hi @BSFishy, @joshuarrrr

Are we good to go with this PR? The branch was recently updated with main bracnh.

BSFishy · 2024-01-02T18:29:46Z

Are we good to go with this PR? The branch was recently updated with main bracnh.

Just need another approval

src-docs/src/views/flex/flex_basis_string.tsx

AMoo-Miki · 2024-01-11T21:17:02Z

src-docs/src/views/flex/flex_basis_string.tsx

+  <div>
+    <OuiFlexGroup>
+      <OuiFlexItem basis={'auto'}>
+        Auto


Because the actual value of the prop we want users to use is auto, we should use auto here and not Auto. Also, we should perhaps wrap it in OuiCode?

The same goes for all of the other values too.

@kgcreative

@AMoo-Miki,

Updated text in examples for Basis in Flex Item for all basis properties (e.g.Auto to auto). Just one question what is this suggestion of wrapping with OuiCode?

src-docs/src/views/flex/flex_example.js

src-docs/src/views/flex/flex_shrink_numeric.tsx

src/components/flex/_flex_item.scss

src/components/flex/flex_item.tsx

Update to new format copyright and license comments at beginning of file Co-authored-by: Miki <[email protected]> Signed-off-by: Samuel Valdes Gutierrez <[email protected]>

Signed-off-by: Samuel Valdes Gutierrez <[email protected]>

…Size and FlexItemShrinkSize types Signed-off-by: Samuel Valdes Gutierrez <[email protected]>

…emShrinkSize deprecated and add a TODO comment to remove them for next major release Signed-off-by: Samuel Valdes Gutierrez <[email protected]>

Signed-off-by: Samuel Valdes Gutierrez <[email protected]>

BigSamu · 2024-01-20T12:51:00Z

@AMoo-Miki, @kolchfa-aws,

Sorry bothering again with this PR, but I would appreciate your feedback on the reviews Mikki sent to see what other changes you need from my side.

Regards,

Samuel

kolchfa-aws · 2024-01-20T17:35:19Z

@BigSamu I made suggestions where Miki flagged the doc team. Let me know if you need any other help from me.

BigSamu · 2024-01-29T14:39:16Z

@BigSamu, I made suggestions where Miki flagged the doc team. Let me know if you need any other help from me.

Thanks @kolchfa-aws,

I will review this week.

@AMoo-Miki,

Any feedback from the suggestions from @kolchfa-aws and also the questions I post on your comments?

Regards,

Samuel

BigSamu · 2024-02-14T14:02:10Z

@AMoo-Miki,

Have you had the chance to review the comments from @kolchfa-aws? I would appreciate your feedback for moving forward on this PR.

Regards,

Samuel

BigSamu added 3 commits October 27, 2023 13:52

adding shrink and basis props for OuiFlexItem

d7426fe

Signed-off-by: Samuel Valdes Gutierrez <[email protected]>

adding tests for shrink and basis props added in OuiFlexItem

0cf7375

Signed-off-by: Samuel Valdes Gutierrez <[email protected]>

changing API for values accepted for basis prop and updating snapshoo…

fe28556

…ts tests for testing Signed-off-by: Samuel Valdes Gutierrez <[email protected]>

BigSamu requested review from seanneumann, AMoo-Miki, rednaksi91, KrooshalUX, BSFishy, bandinib-amzn, ashwin-pc, joshuarrrr, andreymyssak and SergeyMyssak as code owners October 30, 2023 13:31

github-actions bot added the first-time-contributor label Oct 30, 2023

updating CHANGELOG.md and testing snapshoots

d5dd922

Signed-off-by: Samuel Valdes Gutierrez <[email protected]>

BigSamu mentioned this pull request Oct 30, 2023

Add props for shrink and basis for OuiFlexItem #776

Open

BigSamu added 9 commits November 1, 2023 16:30

Merge branch 'main' into feat/props-ouiFlexItem

a4362a8

changing location of chanelog description for this issue into correct…

3eb7413

… place Signed-off-by: Samuel Valdes Gutierrez <[email protected]>

fixing typo lines 122 and 123 flexItem.tsx

428ef63

Signed-off-by: Samuel Valdes Gutierrez <[email protected]>

fixing typo line 55 file _flex_item.scss

eb68e2c

Signed-off-by: Samuel Valdes Gutierrez <[email protected]>

updating jest unit-testing snapshoots

9665bba

Signed-off-by: Samuel Valdes Gutierrez <[email protected]>

changing default values for shrink and basis props

b8ec51e

Signed-off-by: Samuel Valdes Gutierrez <[email protected]>

removing class .ouiFlexItem--flexShrinkZero from _flex_item.scss and …

8abdb38

…flex_item.tsx Signed-off-by: Samuel Valdes Gutierrez <[email protected]>

updating documentation for shrink and basis props in OuiFlexItem

34fad9a

Signed-off-by: Samuel Valdes Gutierrez <[email protected]>

updating jest unit-testing snapshoots

4674627

Signed-off-by: Samuel Valdes Gutierrez <[email protected]>

BSFishy reviewed Nov 2, 2023

View reviewed changes

BSFishy added the OSCI label Nov 2, 2023

removing extra line CHANELOG.md

29aff44

Signed-off-by: Samuel Valdes Gutierrez <[email protected]>

BigSamu added 3 commits November 17, 2023 15:11

Merge branch 'main' into feat/props-ouiFlexItem

e74f52e

Adding tests for combinatios of grow, shrink and basis values and res…

8d6e8fa

…pective snapshoots Signed-off-by: Samuel Valdes Gutierrez <[email protected]>

removing property flex-basis:auto from CSS class ouiFlexItem--flexShr…

c58f11a

…inkZero Signed-off-by: Samuel Valdes Gutierrez <[email protected]>

BSFishy reviewed Nov 20, 2023

View reviewed changes

src/components/flex/flex_item.tsx Outdated Show resolved Hide resolved

src/components/flex/flex_item.tsx Show resolved Hide resolved

src/components/flex/flex_item.tsx Outdated Show resolved Hide resolved

modifiying logic for prop 'basis' according what was requested by mai…

66c8e9e

…ntainers Signed-off-by: Samuel Valdes Gutierrez <[email protected]>

BSFishy approved these changes Dec 5, 2023

View reviewed changes

Merge branch 'main' into feat/props-ouiFlexItem

a539b13

Merge branch 'main' into feat/props-ouiFlexItem

fb97c87