feat: upgrade and rework scroll area #529
Conversation
There was a problem hiding this comment.
In the past, we refactored some directives to components, because the dx feels better with the api.
<hlm-scroll-area> is easier than <ng-scrollbar hlm>.
But I totally agree with the points, that with external dependencies this can be tricky.
I like, that with this approach we are less dependant and it is easier to manage in the future. Additionally, I think it is good, that we don't hide features anymore.
|
@ashley-hunter do you think it's worth the effort to write a migration script? I am trying to figure out if this is a 10 minute thing or 3 hour thing. Because obviously being in alpha is for changes like this! |
Yes, I'm happy to write a migration for this, shouldn't take me too long, just didn't want to spend the time writing one if we weren't happy with this approach, but if we are happy to go forward with this approach I'll gladly write one! |
|
I'd say let's go forward with this. I am wondering if you think we should do this fall all 3rd part libs, e.g. ng-icons being the most prominent of course |
Yes, I think this approach has benefits where we can, could really help avoid issues with breaking changes or libraries not being updated supporting newer angular versions etc.. ng-icons shouldn't be an issue, we can set the size using the CSS variable, and if there are any other issues I can make changes in the library itself to support whatever we need. I'm also working on removing the command component dependency so that will be one less dependency there too. |
|
Added a migration generator/schematic which can be run if necessary |
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?
Closes #510
What is the new behavior?
Does this PR introduce a breaking change?
Other information
This updates the version of ngx-scrollbar. I have also changed the api a bit, however I can revert this if we feel this is not the right approach.
Essentially, there have been a few breaking API changes in the ngx-scrollbar component, removing some of the inputs that were previously available in our wrapper component. The existing spartan component only exposed a small subset of the ngx-scrollbar api and prevented a few of the more advanced features it supports.
With this change we no longer actually rely on the ng-scrollbar component, we just expose a directive to set the recommended styles, in a similar way to how we do with radio buttons etc..
This approach would help us allow the consuming application to use any version they need in future without us needing to update anything (unless the css variable names change) and allows access to all the features of the ngx-scrollbar library, rather than just the few we exposed. It also means if ngx-scrollbar removes or renames any inputs in future this will not be a breaking change on our end hopefully making this easier to manage going forward.
Keen to hear any other opinions on this change.