philmtd / css-fx-layout

A responsive SCSS flexbox library inspired by Angular Flex-Layout.
https://philmtd.github.io/css-fx-layout/
MIT License
48 stars 10 forks source link

"data-flex" doesnt allow for granular control over 'grow' and 'shrink' properties #34

Open drew0530 opened 4 weeks ago

drew0530 commented 4 weeks ago

Issue

The default behavior for using data-flex should be a direct reflection of its CSS counterpart, the flex property. Default behavior is flex: 0 1 auto. Currently, this library has data-flex defaulted to flex: 1 1 auto (see Flex - MDN). You can override the grow property, but then you cannot define the flex-basis, it is set to auto unless it is overridden through another class or with a style attribute declaration.

Solution

While I think that ultimately setting 'flex 0 1 {%}' for data-flex would be the most straightforward answer to this issue, I understand that this library is being used by quite a few people to assist with their migration process, so changing the default behavior of data-flex will likely ruffle some feathers. Because of this, I have a few alternate ideas on how to solve this.

Currently, there is no way to include a flex-basis while also defining the grow/shrink properties. You can only use data-flex="nogrow" (which equates to flex: 0 1 auto) OR data-flex="{percentage}". Using both will just override the first attribute. Again, we have to say style="flex-grow: 0" in conjunction with data-flex="50" in order to produce default behavior.

Examples

a. <div data-flex-intial="50%"></div> b. <div data-flex="noshrink 50%"></div> c. <div data-flex="50" data-flex-nogrow>

I can provide a pull request for these changes, but I wanted to get a sense of direction before I put any real work into applying a solution.

philmtd commented 2 weeks ago

Hi @drew0530, thanks for submitting an issue - and excuse my late reaction due to the holiday season.

Not trying to be nitpicky but I do not think the title of the issue is correct. This library is not causing display issues but is rather not behaving the way you think it should. And I don't think that your ultimate solution would be the most straightforward answer to the issue, either: This library is behaving exactly the way it is intended to - inspired by Angular Flex-Layout, just in pure CSS. That is kind of the "API" that is promised here and I do not intend to break it.

I think of your suggested examples I'd prefer b. so either data-flex="nogrow 50%" or (maybe easier to match in CSS) data-flex="50% nogrow" etc. I think this should be doable similar to other attributes that match more than one term in the attribute value. I am just not sure how to incorporate it with the CSS classes, but am also not sure if it is necessary (even though I initially wanted to keep both ways similar regarding the features).

If you wish to work on a solution (similar to b.), you're welcome to submit a PR. I would just need you to not change any current default behaviour.

drew0530 commented 2 weeks ago

No worries, and I hope the holiday was well for you.

I will work on a pull request as I can find the time to, since my team and I are in the middle of migrating away from flexLayout right now. I'd also like to add that I was wrong in my assumption that flexLayout was using flex: 0 1 as it's defaults, instead of using the default css values, so I was looking in the wrong places to explain why my templates weren't migrating cleanly. Sorry for assuming this project was just randomly setting the grow property to 1!

I will go ahead and change the title of this issue since it doesn't really reflect the fix... which I would actually like to offer another example for; which is from the fxFlex API. I specifically like their 'long-form' vs 'short-form' options, which would look like this:

Short-form (typical usage)

<div data-flex="<basis>"></div>

Long-form (typical usage)

<div data-flex="<grow> <shrink> <basis>"></div>

This way offers the user more granular control over their grow and shrink properties and ultimately should provide a more clean solution for users migrating from this syntax. Let me know what you think and I can get started.

philmtd commented 1 week ago

Sorry, I did not get an email for your edit and only just checked your comment here to see it is much longer now 😅

Thanks. And yes sure, go ahead. I think having a grow-shrink-basis attribute would be a nice addition if you can get it to work alongside the short form. I think I never tried to implement it, but working with the limited abilities of CSS selectors could be a challenge.