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

fix(flex): fill styles #1352

Merged
merged 3 commits into from
May 17, 2019
Merged

fix(flex): fill styles #1352

merged 3 commits into from
May 17, 2019

Conversation

kuzhelov
Copy link
Contributor

@kuzhelov kuzhelov commented May 17, 2019

Fixes #1348

This PR addresses the issue of expected behavior of <Flex fill />, and makes it aligned with the behavior suggested by spec:

fill - Orders container to fill all parent's space available.

BREAKING CHANGES

Effect of this fix is changed behavior of fill prop of Flex component in case if it is a child of other Flex.

<Flex>
  <Flex fill>
  </Flex>
</Flex>

image

If client would like to preserve this look, i.e. inner Flex to take only the minimal width necessary (and not the entire space provided by outer Flex), then

  • fill prop should be removed for inner Flex
  • provide vAlign=stretch prop to outer Flex
<Flex vAlign='stretch'>
  <Flex></Flex>
</Flex>

Description

From https://codesandbox.io/s/ulrbd?module=/example.js :

// outer
<Flex debug .. >
  <Flex fill .. >
     Inner Flex
  </Flex>  
</Flex>

Was

image

Now

image

@codecov
Copy link

codecov bot commented May 17, 2019

Codecov Report

Merging #1352 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@          Coverage Diff           @@
##           master   #1352   +/-   ##
======================================
  Coverage    72.3%   72.3%           
======================================
  Files         764     764           
  Lines        5769    5769           
  Branches     1712    1712           
======================================
  Hits         4171    4171           
  Misses       1592    1592           
  Partials        6       6
Impacted Files Coverage Δ
...eact/src/themes/base/components/Flex/flexStyles.ts 5.26% <ø> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7c68bdf...c43deb6. Read the comment docs.

Copy link
Contributor

@mnajdova mnajdova left a comment

Choose a reason for hiding this comment

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

Would suggest adding a screenshot test for catching possible regressions. Other than that looks good :)

@kuzhelov
Copy link
Contributor Author

@mnajdova, just verified that there is no valid use case for the nested single Flex in Flex - case where, actually, issue was discovered. Would refrain from providing very contrived example in docs for now

@kuzhelov kuzhelov merged commit 58353a0 into master May 17, 2019
@delete-merged-branch delete-merged-branch bot deleted the fix/flex-fill-styles branch May 17, 2019 17:51
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Nested Flex components don't fill correctly
3 participants