CSS: Don't add 'px' to '-ms-flex-order' #1528

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
5 participants
@silverwind
Contributor

silverwind commented Mar 4, 2014

silverwind referenced this pull request in silverwind/droppy Mar 4, 2014

@gibson042

This comment has been minimized.

Show comment Hide comment
@gibson042

gibson042 Mar 4, 2014

Member

I don't want to start adding vendor-prefixed properties to that collection. Why not just define a msFlexOrder cssHook?

Member

gibson042 commented Mar 4, 2014

I don't want to start adding vendor-prefixed properties to that collection. Why not just define a msFlexOrder cssHook?

@mgol

This comment has been minimized.

Show comment Hide comment
@mgol

mgol Mar 4, 2014

Member

@gibson042 I'd imagine using cssHooks is not easy so most people won't realize how to do it. Flex becomes popular so it might be an issue in the future.

I'd like to know how many of those properties there are. If -ms-flex-order is really all we need, I'm for it. If there are significantly more such properties, then -1 from me.

Member

mgol commented Mar 4, 2014

@gibson042 I'd imagine using cssHooks is not easy so most people won't realize how to do it. Flex becomes popular so it might be an issue in the future.

I'd like to know how many of those properties there are. If -ms-flex-order is really all we need, I'm for it. If there are significantly more such properties, then -1 from me.

@scottgonzalez

This comment has been minimized.

Show comment Hide comment
@scottgonzalez

scottgonzalez Mar 4, 2014

Member

Even if cssHooks is too complicated for most users, cssNumber certainly isn't. So even if we don't include this, it's very simple for users to add in their own code.

Member

scottgonzalez commented Mar 4, 2014

Even if cssHooks is too complicated for most users, cssNumber certainly isn't. So even if we don't include this, it's very simple for users to add in their own code.

@silverwind

This comment has been minimized.

Show comment Hide comment
@silverwind

silverwind Mar 4, 2014

Contributor

Aside from order, there are these properties which expect numerical values:

  • -ms-flex-negative
  • -ms-flex-positive

But I'd assume that most people wouldn't actually use these directly, but use the shorthand -ms-flex instead. At least I always do.

Contributor

silverwind commented Mar 4, 2014

Aside from order, there are these properties which expect numerical values:

  • -ms-flex-negative
  • -ms-flex-positive

But I'd assume that most people wouldn't actually use these directly, but use the shorthand -ms-flex instead. At least I always do.

@gibson042

This comment has been minimized.

Show comment Hide comment
@gibson042

gibson042 Mar 4, 2014

Member

Even if cssHooks is too complicated for most users, cssNumber certainly isn't. So even if we don't include this, it's very simple for users to add in their own code.

cssNumber is not part of the public API, but this issue may be just the incentive we need to change that.

Member

gibson042 commented Mar 4, 2014

Even if cssHooks is too complicated for most users, cssNumber certainly isn't. So even if we don't include this, it's very simple for users to add in their own code.

cssNumber is not part of the public API, but this issue may be just the incentive we need to change that.

@dmethvin

This comment has been minimized.

Show comment Hide comment
@dmethvin

dmethvin Mar 4, 2014

Member

Didn't a lot of the properties change from the IE10 draft standard implementation to the one that's currently on track for a standard? I don't think we should be adding properties that are dead-end ones. To me .css() is generally bad practice anyway and this should be done via classes unless there is some extenuating circumstance.

There's already a ticket to document this: jquery/api.jquery.com#164

Member

dmethvin commented Mar 4, 2014

Didn't a lot of the properties change from the IE10 draft standard implementation to the one that's currently on track for a standard? I don't think we should be adding properties that are dead-end ones. To me .css() is generally bad practice anyway and this should be done via classes unless there is some extenuating circumstance.

There's already a ticket to document this: jquery/api.jquery.com#164

@silverwind

This comment has been minimized.

Show comment Hide comment
@silverwind

silverwind Mar 4, 2014

Contributor

Oh, and I almost forgot, but there's also unprefixed versions which probably would need this treatment, I think these should be all:

  • -ms-flex-negative
  • -ms-flex-positive
  • flex-shrink
  • flex-grow
Contributor

silverwind commented Mar 4, 2014

Oh, and I almost forgot, but there's also unprefixed versions which probably would need this treatment, I think these should be all:

  • -ms-flex-negative
  • -ms-flex-positive
  • flex-shrink
  • flex-grow
@silverwind

This comment has been minimized.

Show comment Hide comment
@silverwind

silverwind Mar 4, 2014

Contributor

Didn't a lot of the properties change from the IE10 draft standard implementation to the one that's currently on track for a standard?

Yep, most of what IE10 uses got renamed in the current version of the standard.

To me .css() is generally bad practice anyway and this should be done via classes unless there is some extenuating circumstance.

I totally agree that .css() is bad, but to dynamically sort a list using flexbox ordering, one has to resort to it. For the other properties above thought, I don't really see why they shouldn't be set in the stylesheet directly.

Contributor

silverwind commented Mar 4, 2014

Didn't a lot of the properties change from the IE10 draft standard implementation to the one that's currently on track for a standard?

Yep, most of what IE10 uses got renamed in the current version of the standard.

To me .css() is generally bad practice anyway and this should be done via classes unless there is some extenuating circumstance.

I totally agree that .css() is bad, but to dynamically sort a list using flexbox ordering, one has to resort to it. For the other properties above thought, I don't really see why they shouldn't be set in the stylesheet directly.

@mgol

This comment has been minimized.

Show comment Hide comment
@mgol

mgol Mar 4, 2014

Member

We're definitely missing some flexbox-related properties in that list, even unprefixed ones. We should fix that.

Member

mgol commented Mar 4, 2014

We're definitely missing some flexbox-related properties in that list, even unprefixed ones. We should fix that.

@dmethvin

This comment has been minimized.

Show comment Hide comment
@dmethvin

dmethvin Mar 4, 2014

Member

That would make sense, and quite a few browsers already are shipping it unprefixed: http://caniuse.com/#feat=flexbox

Member

dmethvin commented Mar 4, 2014

That would make sense, and quite a few browsers already are shipping it unprefixed: http://caniuse.com/#feat=flexbox

@silverwind

This comment has been minimized.

Show comment Hide comment
@silverwind

silverwind Mar 4, 2014

Contributor

So the question is, which ones should I add on the pull request?

This is the full list:

  • flex-shrink
  • flex-grow
  • -ms-flex-order
  • -ms-flex-negative
  • -ms-flex-positive

The first three are the minimum, I'd say. flex-shrink and flex-grow obviously because they're standardized, -ms-flex-order for the necessity of list-sorting. -ms-flex-negative and -ms-flex-positive really don't seem of any use to me.

Contributor

silverwind commented Mar 4, 2014

So the question is, which ones should I add on the pull request?

This is the full list:

  • flex-shrink
  • flex-grow
  • -ms-flex-order
  • -ms-flex-negative
  • -ms-flex-positive

The first three are the minimum, I'd say. flex-shrink and flex-grow obviously because they're standardized, -ms-flex-order for the necessity of list-sorting. -ms-flex-negative and -ms-flex-positive really don't seem of any use to me.

@mgol

This comment has been minimized.

Show comment Hide comment
@mgol

mgol Mar 4, 2014

Member

I wouldn't us cherry-pick which prefixed versions to support and which not, it will be confusing to the user; way more than us just not supporting prefixed versions in this way at all.

The problem here is the user doing $div.css('-ms-flex-order', 2) will not expect anything can break from that. OTH, if we start adding prefixed properties, we're in for a potentially big list.

I wish we didn't add px automatically at all but changing that would be a compatibility break, potentially huge.

Member

mgol commented Mar 4, 2014

I wouldn't us cherry-pick which prefixed versions to support and which not, it will be confusing to the user; way more than us just not supporting prefixed versions in this way at all.

The problem here is the user doing $div.css('-ms-flex-order', 2) will not expect anything can break from that. OTH, if we start adding prefixed properties, we're in for a potentially big list.

I wish we didn't add px automatically at all but changing that would be a compatibility break, potentially huge.

@mgol

This comment has been minimized.

Show comment Hide comment
@mgol

mgol Mar 4, 2014

Member

If we can do it via exposing cssNumber instead of forcing users to resort to cssHooks, that seems more user-friendly.

Member

mgol commented Mar 4, 2014

If we can do it via exposing cssNumber instead of forcing users to resort to cssHooks, that seems more user-friendly.

@dmethvin

This comment has been minimized.

Show comment Hide comment
@dmethvin

dmethvin Mar 10, 2014

Member

@dmethvin dmethvin closed this Mar 10, 2014

@mgol

This comment has been minimized.

Show comment Hide comment
@mgol

mgol Mar 10, 2014

Member

@dmethvin we still need to add standard flex properties to the list and I was under the impression OP wanted to do it. Shall we reopen?

Member

mgol commented Mar 10, 2014

@dmethvin we still need to add standard flex properties to the list and I was under the impression OP wanted to do it. Shall we reopen?

@dmethvin

This comment has been minimized.

Show comment Hide comment
@dmethvin

dmethvin Mar 10, 2014

Member

Oh that's cool, but a fresh PR probably makes more sense because at this point it's not related. @silverwind we'd love that PR for the standard properties when you get a chance.

Member

dmethvin commented Mar 10, 2014

Oh that's cool, but a fresh PR probably makes more sense because at this point it's not related. @silverwind we'd love that PR for the standard properties when you get a chance.

@mgol

This comment has been minimized.

Show comment Hide comment
@mgol

mgol Mar 10, 2014

Member

@dmethvin Yeah, that makes sense.

Member

mgol commented Mar 10, 2014

@dmethvin Yeah, that makes sense.

@silverwind

This comment has been minimized.

Show comment Hide comment
@silverwind

silverwind Mar 10, 2014

Contributor

@dmethvin Alright, I'll provide a new PR shortly.

Contributor

silverwind commented Mar 10, 2014

@dmethvin Alright, I'll provide a new PR shortly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment