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

Added grow-shrink-basis as optional properties to flex-sizes #35

Closed
wants to merge 1 commit into from

Conversation

drew0530
Copy link

Modifications suggested as a part of this issue thread: #34

@philmtd
Copy link
Owner

philmtd commented Sep 12, 2024

Thanks for your PR!

After reviewing the code, I am not sure the syntax is ideal. Without looking into the implementation or documentation, it is suggesting you may use other numbers than 0 or 1 for grow/shrink. I think this may confuse some users.

Do you think we could find another way of doing it? Maybe creating new selectors like data-flex-grow and data-flex-shrink with just the flex basis as the value instead? I'm open to other suggestions and your input.

@drew0530
Copy link
Author

A few things:

  1. I'm failing to see what part might be confusing to users, the syntax outlined here matches the syntax I provided to you and you accepted in the issue linked above, and is 1:1 to the long-form that was present in the fxFlex API.
  2. fxFlex API does provide the option to define more values than 1 or 0 for grow and shrink, however implementing this would require either an exponential increase to the code generated whenever 'flex-size-attribute' is called because I have no way of knowing how many possible values to include for grow and shrink. The only way I could see implementing this would work would be to include a similar system to the 'flex-size-attributes' @mixin, where we define 'from', 'to', and 'increment' and then the user has to include an additional @include in their initial setup, which is, in my opinion, more confusing to the user and creates another layer of complexity for starting a new project.
  3. The suggestion you made of creating new selectors like data-flex-grow and data-flex-shrink with just the flex basis as the value instead comes with its own set of issues. I cannot see a way (outside of marking the properties as '!important') of ensuring that the default grow and shrink values are overridden from <div data-flex>. Not to mention that it would require a completely different approach for a solution to the short vs. long format that I cleared with you.

Additionally to all of this, I was very specific as to how I was going to implement the changes, and waited to make them until I was sure you were on board with the way they would look. I can see how you might want to add additional flexibility for more values with grow and shrink to be more complete, but until this issue was raised there wasn't a way to define them at all unless you didn't care about also defining your flex-basis. I believe that overall, the change I provided includes more functionality than was previously offered to the users (without affecting existing code), follows the same syntax as fxFlex, and does so with minimal additional generated code.

@philmtd
Copy link
Owner

philmtd commented Sep 13, 2024

Look, I'm sorry you're frustrated, and apparently we had a misunderstanding.

  1. An API that suggests that it may be used like the library this one attempts to replace but is limited in an intransparent way is confusing and I do not intend to add such code here. I rather have less functionality than intransparent limitations. After all, this code needs to be maintained (very likely by me) if issues arise in the future.
  2. What I thought we agreed on (without explicitly specifying it, I admit) was that we would find a way to make all variables in the value work. One, or the only (?) possible solution to this would be mixins, like you said. I agree that this is another kind of confusion, but one that already exists within this library and one that is very explicit. If a user starts using css-fx-layout, they already do have to read the documentation to configure their desired selectors. One more mixin will not change that complexity, in my opinion, even less so for a feature that is only now requested after this library existed for some years and, as such, most users probably don't need.
  3. Yes, that's true. Maybe the idea with additional selectors was not that good after all.

To be clear with my ideas now: If you really want these selectors to be part of this library, please implement them as a configurable mixin with documentation and everything. I know that this is a lot of work to do, but that is the way that would comply best with how everything here already works. I will not merge anything other than that.

If this is not something you can agree with or you can't or don't want to invest this much time into it (which I fully respect and understand), then I will recommend you implement these selectors in your application's code. It should be as easy as copying and pasting your added lines plus the mixin around it into your own SCSS and applying that mixin next to the provided css-fx-layout mixins. Or you maintain a fork of this project that you can use for your purposes.

Again, I'm sorry if this caused frustration. I understand from our conversations that you value your time a lot. If you don't have the time for a thorough review process with suggested changes and discussions, which can always include another iteration of changes, I recommend you close the PR and just implement these selectors in your application. I also only have limited time and am not getting paid for working on this project, so please understand that I am careful when it comes to adding features.

@drew0530
Copy link
Author

Sorry for my late response and my sharp response. I agree that with you that having this fully compatible with all values given to it is the more complete solution, and I understand where you're coming from as far as LTS. I also agree with you on the point made about complexity, considering that the user will already be familiar with this system for setup. I have a few things that I need to take care of at work this week and possibly into the next, but I will take a look when I get free time into trying to add a solution that we discussed in (2). At the end of the day, this project has been a huge time saver for me and my team and I'm very thankful for all the work you've put into it, so I enjoy putting the work into extending its functionality.

@philmtd
Copy link
Owner

philmtd commented Oct 2, 2024

Hey @drew0530, thank you for your response, and I'm happy to hear this. Sure thing, take your time. Should you have any questions, don't hesitate to ask them. I'll try to reply as quickly as I can.

@philmtd
Copy link
Owner

philmtd commented Mar 5, 2025

I'm closing this, since it is inactive.

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.

2 participants