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

We do have to expand flex values #56

Closed
wants to merge 12 commits into from
Closed

Conversation

iegik
Copy link

@iegik iegik commented Nov 6, 2018

flex: 30px means flex: 0 0 30px in IE10 instead of flex: 0 1 30px, so we do have to expand it 😕

Check out the updated Flexbug 6.
#26 (comment) by @silvenon

Issue: #26

@iegik iegik changed the title Fix #48 We do have to expand flex values Nov 9, 2018
@silvenon
Copy link
Contributor

silvenon commented Nov 9, 2018

I've been out of the flexbugs game for a long time, I'll double-check this on IE 10 tomorrow and get back to you.

@axelboc
Copy link

axelboc commented Nov 12, 2018

I don't think expanding the shorthand is the right approach. If I understand issue #26 correctly, the idea is to set explicitly the value of flex-shrink to 1 inside the flex shorthand when not specified:

/* IGNORE (flex-shrink already specified) */
flex: 1 1;
flex: 1 1 100%;
flex: 100% 1 1;

/* FIX */
flex: 1; /* => flex: 1 1 0%; */
flex: 1 10%; /* => flex: 1 1 10%; */
flex: 2em; /* => flex: 0 1 2em; */

Copy link
Contributor

@silvenon silvenon left a comment

Choose a reason for hiding this comment

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

I think the only way for this plugin to confidently move forward is to add e2e tests (e.g. Sauce Labs) for IE 10/11 and Safari 9/10 to double-check everything. I'll try to integrate those in another PR because I can't keep flexbugs in my head.

Also, I can't remember why I posted that comment you cited and what I meant by it. 😕

@@ -3,7 +3,7 @@ var test = require('./test');
describe('bug 6', function() {
it('Set flex-shrink to 1 by default', function(done) {
var input = 'div{flex: 1;}';
var output = 'div{flex: 1 1;}';
var output = 'div{flex-grow: 1;flex-shrink: 1;}';
Copy link
Contributor

Choose a reason for hiding this comment

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

I think flex: 1 should become flex: 1 1 0% (or flex: 1 1 0, I can't remember if and why unitless is better) because flex: 1 also sets flex-basis, so this fix isn't correct.

Copy link
Author

@iegik iegik Nov 13, 2018

Choose a reason for hiding this comment

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

IE11 do not understand unitless :(

If the is ‘0’, it must be specified with a unit (like ‘0px’) to avoid ambiguity; unitless zero will either be interpreted as as one of the flexibilities, or is a syntax error.

This is no longer true in the spec, but IE 10-11 still treat it as true. If you use the declaration flex: 1 0 0 in one of these browsers, it will be an error and the entire rule (including all the flexibility properties) will be ignored

https://github.com/philipwalton/flexbugs#flexbug-4

@@ -17,12 +17,17 @@ describe('bug 6', function() {
});
it('when flex-shrink is set explicitly to zero', function(done) {
var css = 'div{flex: 1 0 0%;}';
var output = 'div{flex: 1 0;}';
var output = 'div{flex-grow: 1;flex-shrink: 0;flex-basis: 0;}';
Copy link
Contributor

Choose a reason for hiding this comment

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

Like I said, I can't remember why this should be expanded, same applies to the rest of your changes…

@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

3 participants