oui

oui copied to clipboard

Is your feature request related to a problem? Please describe.

OuiFlexItem only supports the grow property, while shrink and basis are also important and useful properties. There are some cases where they are needed to achieve certain layouts.

Describe the solution you'd like

Add shrink and basis properties for OuiFlexItem. These should be similar to grow but for flex-shrink and flex-basis.

Describe alternatives you've considered

Using custom HTML is an option but is extremely not preferred.

Additional context

This gap was identified in OUI compliance audits:

• https://github.com/opensearch-project/OpenSearch-Dashboards/issues/3966
• https://github.com/opensearch-project/OpenSearch-Dashboards/issues/4122
• https://github.com/opensearch-project/OpenSearch-Dashboards/issues/4162

@joshuarrrr @BSFishy I would like to work on this issue

@BSFishy, @joshuarrrr,

I think I am almost done with this issue. However, I have some questions if you can help me with:

• How can I perform snapshot testing with the changes I added in OuiFlexItem component when I added the props shrink and basis with its respective logic? A new snapshot has to be created first with Jest? Not sure how to proceed here.

• When asking to add the props shrink and basis to be similar to the props grow, the latter has a case where it accepts booleans values, apart from number values from 1 to 10. When grow=true this means: "please let this OuiFlexItem element grow" (inherits this behaviour from OuiFlexGroup component -> flex-grow:1 and flex-basis: 0%). On the other hand, when grow=false this means: "don't allow this OuiFlexItem element to grow" (flex-grow: 0 and flex-basis: auto). Two questions here:

  • For the case of the shrink prop, I replicated the formula accepting true and false values and number for 1 to 10. Is this OK with you? There are notflex-shrink settings coming from the OuiFlexGroup component.
  • For the case of the basis prop, I am not considering a formula for accepting true or false values here. What I only accept is a string value that accepts only the values: auto, 0%, 25%, 50%, 75% and 100%. Is this Ok with you? Or should we add more options here?

  Here a link with the commits I recently did.

  • Commit 0cf737
  • Commit d7426f

• How can I perform snapshot testing with the changes I added in OuiFlexItem component when I added the props shrink and basis with its respective logic? A new snapshot has to be created first with Jest? Not sure how to proceed here.

Snapshots are generated by the unit tests. So generally, we'll have a component file component.tsx then a test file component.test.tsx, then those tests will have lines something like expect(component).toMatchSnapshot(). Jest will interpret that, generate snapshots if they don't exist, then compare what it got to the snapshot. From your commits, it looks like this is what you did.

• For the case of the shrink prop, I replicated the formula accepting true and false values and number for 1 to 10. Is this OK with you? There are notflex-shrink settings coming from the OuiFlexGroup component.

Yes, the code you linked looks good to me.

• For the case of the basis prop, I am not considering a formula for accepting true or false values here. What I only accept is a string value that accepts only the values: auto, 0%, 25%, 50%, 75% and 100%. Is this Ok with you? Or should we add more options here?

For this one, I think accepting true, false, 'auto', then keep it restrictive on the API we provide for now, with 'max-content', 'min-content', and 'fit-content'. From there, if we want to expand what this prop supports in the future, we always can.

Snapshots are generated by the unit tests. So generally, we'll have a component file component.tsx then a test file component.test.tsx, then those tests will have lines something like expect(component).toMatchSnapshot(). Jest will interpret that, generate snapshots if they don't exist, then compare what it got to the snapshot. From your commits, it looks like this is what you did.

Thanks, @BSFishy. I will take a look at the testing to understand it better and pass all unit tests.

Yes, the code you linked looks good to me. Great!

For this one, I think accepting true, false, 'auto', then keep it restrictive on the API we provide for now, with 'max-content', 'min-content', and 'fit-content'. From there, if we want to expand what this prop supports in the future, we always can.

For this case @BSFishy, why should we have cases for true or false values for this prop? In other words, what is supposed to mean a prop basis=true or basis=false? Same case for shrink=true or shrink=false. If you check the Sass file, this will not have any effect. Same question for the case grow=null, shrink=null, basis=null. If a user passes a parameter like that, this will not affect the flex styles of the OuiFlexItem. Here, I just follow the logic for type FlexItemGrowSize when creating the type FlexItemShrinkSize

export type FlexItemShrinkSize =
  | 1
  | 2
  | 3
  | 4
  | 5
  | 6
  | 7
  | 8
  | 9
  | 10
  | true
  | false
  | null;

Also, to just check my understanding of what we should accept for the basis prop in the API, the array below is the one you expect in the code?

export const BASIS_VALUES: FlexItemBasisValue[] = [
  'auto',
  'max-content',
  'min-content',
  'fit-content',
];

Thanks,

Samuel

@BSFishy,

PR #1126 was open for this issue for your 1st review. I haven't updated the documentation yet. Wondering about the guidelines to follow to add the props shrink and basis. Lets discuss that in the PR itself.

Regards,

Samuel