Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix initial and auto value for flex #54

Closed
wants to merge 26 commits into from
Closed

Conversation

iegik
Copy link

@iegik iegik commented Oct 31, 2018

This PR to handle the initial and auto value for flex CSS property

flex: initial → flex: 0 1 auto
flex: auto → flex: 1 1 auto

#48

Default value:	0 1 auto
@iegik
Copy link
Author

iegik commented Oct 31, 2018

tests: #55

@luisrudge
Copy link
Owner

Hi, thanks for the PR. Can you add a bit of context on this change? 😬

This says the default is 0

@iegik
Copy link
Author

iegik commented Nov 1, 2018

Here https://github.com/luisrudge/postcss-flexbugs-fixes/pull/54/files#diff-4346a0d83ae11ca744ad6aed47d5e8fcL4 is all about flex css property, not 'flex-basis`.

flex: initial - This is the default value. The item is sized according to its width and height properties. It shrinks to its minimum size to fit the container, but does not grow to absorb any extra free space in the flex container. This is equivalent to setting flex: 0 1 auto.

flex-basis: <length> | auto; /* default auto */

@iegik
Copy link
Author

iegik commented Nov 1, 2018

Ok, I leave default value to 'auto', but also leave 0 if it appears.

@iegik
Copy link
Author

iegik commented Nov 1, 2018

@luisrudge
Copy link
Owner

I guess this conflicts with #22

@silvenon what do you think about this PR? 🤔

@silvenon
Copy link
Contributor

silvenon commented Nov 2, 2018

Initial value is not the same as default value. The default value for flex-basis is the one being set when you're using the flex shorthand without covering flex-basis, initial value is what was there initially.

  • flex: initialflex: 0 1 auto
  • flex: 1flex: 1 1 0%

Initial values are outside of the control of the plugin because it can work only on present rules, not absent ones, unless someone explicitly writes something like flex: initial. Maybe you could add a transformation for that and modify the readme to recommend globally setting flex: initial instead of flex-basis: 1. I don't know if this plugin already has that transformation, it might be handy.

@iegik
Copy link
Author

iegik commented Nov 6, 2018

@silvenon what about "Safari seems to hate '0%' and the others seems to make do with a nice value when basis is missing" ?
IE11 waits 3 props, not 2. ('flex: 1 → flex: 1 1')

Should we normalize flexBasis to 0?

@luisrudge
Copy link
Owner

@iegik are you talking about this issue? #48

If so, let's fix both scenarios but in different PRs. Please update this PR to handle only the 'initial' value (and add a test for it), and let's create another PR that fixes #48

Thanks 🎉

@luisrudge
Copy link
Owner

@iegik thanks! can you add a test for this, please? 🎉

@iegik iegik changed the title Fix default value for flexBasis Fix default value for flex Nov 6, 2018
@iegik iegik changed the title Fix default value for flex Fix initial value for flex Nov 6, 2018
@iegik
Copy link
Author

iegik commented Nov 9, 2018

breaks bug 4, 8.1.a

index.js Outdated
@@ -3,7 +3,7 @@ var bug4 = require('./bugs/bug4');
var bug6 = require('./bugs/bug6');
var bug81a = require('./bugs/bug81a');

var doNothingValues = ['none', 'auto', 'content', 'inherit', 'initial', 'unset'];
Copy link
Author

Choose a reason for hiding this comment

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

Bad practice to declarate rule in index.
These checks chould be repeated in each ./bugs/bunny file!

@iegik iegik changed the title Fix initial value for flex Fix initial and auto value for flex Nov 9, 2018
@axelboc
Copy link

axelboc commented Nov 12, 2018

@iegik I see you've renamed the PR to focus on flexbug #8, but the code still seems to do a lot more. May I ask where you're at? It's now a fairly simple problem, which shouldn't affect the other bugs. Let me know if you need a hand ;)

@iegik
Copy link
Author

iegik commented Nov 13, 2018

@axelboc No, this PR is focused to FlexBug #6

  • added default values for initial and auto
  • removed tests, that not affects to this bug

The problem now is, that changing default flex values in bug 6 also affects to bug 4 and 8.1.a tests.

@iegik
Copy link
Author

iegik commented Nov 13, 2018

I think, that this PR and #56 should be merged as one PR

@adrienharnay
Copy link

Hey, wouldn't this PR revert the fix mentioned in #45 and #46 ?

@iegik iegik closed this Feb 3, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants