fix: code quality linting improvements #550
Conversation
| > | ||
| <hlm-checkbox-checkicon [class]="checkIconClass()" [iconName]="checkIconName()" /> | ||
| <ng-icon hlm size="sm" name="lucideCheck" /> |
There was a problem hiding this comment.
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.
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.
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.
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.
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.
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!
Co-authored-by: Ajit Panigrahi <[email protected]>
Co-authored-by: Ajit Panigrahi <[email protected]>
PR Checklist
Please check if your PR fulfills the following requirements:
guidelines: https://github.com/goetzrobin/spartan/blob/main/CONTRIBUTING.md#-commit-message-guidelines
PR Type
What kind of change does this PR introduce?
Which package are you modifying?
What is the current behavior?
N/A
What is the new behavior?
Following the discussion on discord earlier where linting errors may occur in an Nx repo, I have went through our code and fixed some areas where eslint was disabled due to things like poor types etc.. This is a pre-cursor to fixing the reported issue, but should reduce the custom settings required to be any shared config we ship.
Fixing an eslint config using old format.
I have also removed a checkbox icon component as it can be inlined to reduce amount of code required - and we previously had icon class and icon name inputs for the checkbox. These don't seem very useful and normally the check icon would be the same in all checkboxes. In the rare case that this would need to be changed, the code can be modified in the project as it is a helm component, but it seems cleaner not to pollute to component API for a very rare case that has a simple workaround - I can revert if there is a valid case Im not aware of.
Does this PR introduce a breaking change?
This removes the checkbox icon and checkbox icon class inputs. Typically every checkbox in an app should have the same icon, so customization should be done within the helm component rather than having to do it on every checkbox in an app. I can revert this is we feel its really beneficial, but no other ui libraries seem to offer this so it seems unlikely to have widespread usage.
Other information