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

Confusing times #57

Closed
axelboc opened this issue Nov 12, 2018 · 4 comments
Closed

Confusing times #57

axelboc opened this issue Nov 12, 2018 · 4 comments

Comments

@axelboc
Copy link

axelboc commented Nov 12, 2018

The state of this plugin is really confusing.

  1. How is outputting flex: 1 1 solving flexbug #4? Am I missing something?

  2. I don't understand why 3.3.0 breaks latest Chrome #45 led to the re-revert of Revert Autoremoval of 0% Basis #43, when the issue seems to be talking about cases where the flex-basis value is placed first in the flex shorthand -- e.g. flex: 100%, flex: 100% 2. Seems to me like a pretty specific case that could be dealt with separately.

  3. I've seen mentioned that Safari doesn't support 0%, but isn't this flexbug #11, which can only be fixed with some more advanced, case-by-case refactoring?

In my opinion, this plugin should not worry about outputting code that breaks some non-trivial cases (e.g. min/max-width, flex-wrap, etc.) If flex: 1 1 0% breaks something, it means the developer should not have written a simple flex: 1 in the first place. How about leaving the transforms as dumb as possible? If I do use flex: 1 for a basic flex layout, I'd really like to get flex: 1 1 0% -- no questions asked.

@axelboc
Copy link
Author

axelboc commented Nov 12, 2018

More to the point, here is how I think the existing transform for flexbug #4 should work. The bug refers specifically to a flex-basis value of 0 or 0px in the flex shorthand, which should both be transformed to 0%:

/* IGNORE */
flex: 1; /* no flex-basis */
flex: 2 0; /* no flex-basis */
flex: 1 0 50px; /* valid unit */
flex: 0 0 0%; /* valid unit */
flex: 10px; /* valid unit */
flex: 2em 3; /* valid unit */
flex: 0% 2 0; /* valid unit */
flex: 1 1 1; /* invalid */
flex: 1% 1 1%; /* invalid */

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

I haven't checked but if IE doesn't support the <flex-basis> <flex-grow> <flex-shrink> syntax, the re-ordering could be handled by a separate transform.

@luisrudge
Copy link
Owner

I agree that's a better solution to bug4. Can you send a PR?

@axelboc
Copy link
Author

axelboc commented Nov 13, 2018

This is going to take a bit of planning. I see two big issues at the moment:

  1. We don't have a way to consistently and robustly parse the value of the flex shorthand. This is important because:
    • processing invalid or complex values is most likely a bad idea (e.g. two flex basis: 0% 1 0%, four values: 0 0 0 0, custom properties: var(--grow)`, ...);
    • the shorthand's syntax is not trivial (e.g. flex-basis first) and attempting to process it in specific ways inside each bug will lead to actual bugs and inconsistencies.
  2. The bugs are not tested in isolation; they're only tested together, so there's no way to tell which one does what. For instance, the README says that bug 4 rewrites flex: 1 to flex: 1 1 when, in fact, it rewrites it to flex: 1 1 0%; bug 6 is the one that rewrites this value again to 1 1.

I think a big rewrite of bugs 4 and 6 is in order and I'd be happy to have a go at it. I'm just not sure how to do this smoothly so that stuff don't break or conflict with other work. The way I see it, the smoothest path might be as follows:

  1. Test each bug in isolation. This would affect neither the current behaviour of the bugs nor the existing tests, except perhaps for some restructuring and tidying (I've seen a lot of outdated test descriptions).
  2. Implement a flex shorthand parser, perhaps using something like this CSS dimension parser.
  3. Refactor bug 4.
  4. Refactor bug 6.

Any thoughts?

@axelboc
Copy link
Author

axelboc commented Nov 30, 2018

Closing this for now. Too much work, too little time :'(

@axelboc axelboc closed this as completed Nov 30, 2018
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

No branches or pull requests

2 participants