-
Notifications
You must be signed in to change notification settings - Fork 59
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
Expand shorthand flex syntax for flexbox for IE #166
Expand shorthand flex syntax for flexbox for IE #166
Conversation
cc: @ljharb @garrettberg |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems reasonable overall
modules/plugins/flexboxIE.js
Outdated
if (property === 'flex') { | ||
// For certain values we can do straight mappings based on the spec | ||
// for the expansions. | ||
if (flexShorthandMappings.hasOwnProperty(value)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should probably use Object.prototype.hasOwnProperty.call, for safety
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can certainly update it, was following the prior syntax already in place a few lines above at https://github.com/rofrischmann/inline-style-prefixer/pull/166/files#diff-1aebbd1a101132a004e54fd39eead4fbR33.
modules/plugins/flexboxIE.js
Outdated
// always be a unitless number and represents flex-grow. | ||
// The second unit will represent flex-shrink for a unitless | ||
// value, or flex-basis otherwise. | ||
if (isPositiveNumber.test(flexValues[1])) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would parseFloat(flexValues[1]) >= 0
not work here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't believe so, as for example parseFloat('2px')
will equal 2
, and we are looking for unitless, since parseFloat('2')
parses to the same thing and represents flex-shrink
, while the former value of 2px
will represent flex-basis
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this does call out an issue though in that it's not handling floats, so something like flex: 1.1
should map to flex: 1.1 1 0%
, which this implementation isn't doing. Will update the matching to account for floats, thanks for pointing this out!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
So it's ready to merge right? |
Yep, should be ready to go! |
(y) Thanks for the work =) |
Fixes #165. This logic is based off of the documentation at https://developer.mozilla.org/en-US/docs/Web/CSS/flex, and some of the expected test values are from pasting some sample values into dev tools and seeing the computed results as a sanity check.
I tried to make this reasonable, the flex specs are not overly complex but can get a bit interesting in the shorthand syntax with unitless and united values. Please let me know if you think this is a reasonable approach.