fix: select fixes and cleanup #614
Open
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Love the change, yea I think at initial concept leveraging CDK Listbox seemed like a good idea but it ultimately didn't work to well in the end. Im excited for this! |
Thank you! The changes I believe should now be complete, if I have missed anything let me know 😀 thanks! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
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 #453, #551
What is the new behavior?
I have reworked the select component, previously we have 3 sources of truth, the user state, the select service state and the CDK listbox directives state. Keeping this all in sync is a bit of a nightmare, and while utilizing the cdk usually reduces the amount of code required, in the case it actually increased it by quite a lot, since we don't actually use much of the functionality it provides. This PR refactors the component, simplifying it greatly and allowing us to avoid the bugs that previously existed due to this inconsistent state.
I have also added support for custom value and placeholder templates.
Does this PR introduce a breaking change?
There are two minor breaking changes in this PR, the first is I have renamed openedChange, to openChange, this allows us to two way bind the property, and follows the typical naming convention.
The second is the switch from focusing each option to using active descendant. Active descendant is much simpler than changing tab indexes and ensuring that the if the focused item is removed that everything is updated correctly. This is how libraries like Material handle keyboard navigation in select components. The only real breaking changing is the focus styling class in helm option needs slightly changed.
Both of these breaking changes have migrations and health check to automatically migrate them.
Other information