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

feat(Flex): replace Flex.Gap with adding margins on the elements #1074

Merged
merged 15 commits into from
Mar 21, 2019

Conversation

mnajdova
Copy link
Contributor

@mnajdova mnajdova commented Mar 19, 2019

Fixes the issue with rendering double gap between elements if the one between them is hidden. Does not render additional element for the gap => reduced amount of rendered dom elements.

Example code:

import React from 'react'
import { Flex, Input, Button, Segment } from '@stardust-ui/react'

const FlexExampleInput = () => (
  <Flex gap="gap.medium" debug>
    <Segment content='Segment' />
    <Segment content='Segment' styles={{display: 'none'}} />
    <Segment content='Segment' />
    <Segment content='Segment' />
  </Flex>
)

export default FlexExampleInput

Previously:

image

Now:

image

@codecov
Copy link

codecov bot commented Mar 19, 2019

Codecov Report

Merging #1074 into master will decrease coverage by 0.01%.
The diff coverage is 60%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1074      +/-   ##
==========================================
- Coverage   81.78%   81.77%   -0.02%     
==========================================
  Files         702      701       -1     
  Lines        8580     8564      -16     
  Branches     1245     1170      -75     
==========================================
- Hits         7017     7003      -14     
+ Misses       1548     1546       -2     
  Partials       15       15
Impacted Files Coverage Δ
...act/src/themes/teams/components/Flex/flexStyles.ts 10% <0%> (+0.9%) ⬆️
packages/react/src/components/Flex/Flex.tsx 100% <100%> (ø) ⬆️

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 a1f58c7...3e20271. Read the comment docs.

}
},
...(p.gap && {
'> *:not(:last-child)': {
Copy link
Contributor Author

@mnajdova mnajdova Mar 19, 2019

Choose a reason for hiding this comment

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

I tried several options for adding these styles.

  • adding marginLeft and marginTop on all elements expect the first one (this however was conflicting with the push prop on the Flex.Item, as it should override the same styles, but is applied first..)
  • I tried applying the margins which are not used (the marginBottom and marginRight), but with this I had to find figure out which element in the Flex is the last one which can be kind a tricky (we can have null values, or even the children in the Flex.Item can be values - it seemed kind a risky...)
  • adding gap property on the Flex.Item, which will just be propagated, and the inside the flexItemStyles I was adding these margins before the styles of the push prop were provided). For the other child components inside the Flex, I was providing the same styles. This introduced lots of duplication around the gap styles, and I didn't want to add new prop to the Flex.Item just because of the internal implementation.

That's how, I ended up with this, which seems to be the most simple solution, but I had to use this specific selector, which means it will always win over some inline styles, which may be surprising for the users... Let me know if you have some other ideas.

Copy link
Contributor

Choose a reason for hiding this comment

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

let me, please, just point out couple of problems that we would need still address - but should mention explicitly that these were the problems of the original approach with Flex.Gap as well. Both these problems appear when wrap prop is truthy for Flex. Lets suppose that flex direction is row, wrapping is happening. Then

  • there is an extra gap on the right side of last element on the line - ideally we would like to omit it
    image

  • *there is no gap between flex lines - probably, eventually we might want to address this scenario

@mnajdova mnajdova changed the title [WIP] feat(Flex): replace Flex.Gap with adding margins on the elements feat(Flex): replace Flex.Gap with adding margins on the elements Mar 19, 2019
@mnajdova
Copy link
Contributor Author

For some reason one of the examples for the Flex is difference, very minimal difference between the Flex.Items when the size prop is used.. (Anybody knows why is this happening?)

CHANGELOG.md Outdated Show resolved Hide resolved
Co-Authored-By: mnajdova <mnajdova@gmail.com>
@mnajdova mnajdova merged commit 087d913 into master Mar 21, 2019
@delete-merged-branch delete-merged-branch bot deleted the fix/flex-gap branch March 21, 2019 13:06
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants