-
Notifications
You must be signed in to change notification settings - Fork 11
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
Conversation
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 |
A few things:
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. |
Look, I'm sorry you're frustrated, and apparently we had a misunderstanding.
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. |
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. |
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. |
I'm closing this, since it is inactive. |
Modifications suggested as a part of this issue thread: #34