Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
fix: code quality linting improvements #550
fix: code quality linting improvements #550
Changes from 2 commits
d4864d4
f1e6679
9b078ee
0a9b676
d603435
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
This file was deleted.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The name should come from the input, some users want to use another icon.
I see that you removed the input. I would say this is a breaking change.
Would you say, if someone wants another icon he should change the hlm source in his repo?
This also applies to the class
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, so I guess the question is, where does it make sense for this kind of customization? Is there really a common case for checkboxes in different parts of an app to have a different check icons?
In most cases that I have seen e.g. material, shadcn etc.. the icon is always the same for every checkbox.
I can see a case where a user may want a different icon for every checkbox, in which case customizing the helm component makes perfect sense. But does it make sense to also change it on a per instance basis?
If so I can certainly add it back in, happy to hear thoughts!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I cannot remember why the inputs were there in the first place.
I just wanted to point out that some users could maybe used this feature.
Just an example that pops into my head. Someone used checkbox in a tic tac toe game. Maybe not the best example. 😉 But we don't know what users are coming up with when using the library.
In an normal app i totally agree with you, that checkboxes should always look the same, and maybe these special cases should not be handled with a checkbox.
Both ways have their valid points and I have a hard time to decide which one is better.
Just another question. Who provides the icon now, that was previously done by the hlmcheckicon component, which also was a little bit weird if someone used his own icon.
So maybe the cleaner end result is to remove it and document it as breaking change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The helm checkbox icon component was only used internally so never something that would have been used by a consumer.
The checkbox component now provides the icon directly.
But yes in the case where someone was to provide a custom icon they would have to provide it manually themselves.
I'm happy to restore it if we think it's worth it, just not something any other library I see provides out of the box and most libraries don't copy the components into the project, we do, so that kind of infrequent thing could be added in the project if someone needed it, rather than us needing to provide it by default for everyone, but I can easily restore it if we think it's best.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh i missed that!
It is fine for me, if someone needs the inputs he still has the possibility to do this in his own helm component.
I believe it is the cleaner way, but it has the possibility to be a breaking change.
Nice work
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Grand, I have updated the PR description to mention this. If anyone else has an opinion let me know, happy to revert this if needed. Thanks for you feedback!