-
-
Notifications
You must be signed in to change notification settings - Fork 165
Replace Choices.js
with slim-select
#8351
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
base: 5.5
Are you sure you want to change the base?
Conversation
I would do it as a bugfix for 5.5 though - as it's already broken there anyway. |
We could do so as there is not yet a possibility to change the configuration but we would need to make sure that libraries using the controller would not break (e. g. having Wdyt about that @leofeyer ? |
I lean towards "new feature for Contao 5.6". But the current implementation is also broken enough for a bugfix 😄 |
Styles have been adjusted in f4db717 |
static values = { | ||
options: { | ||
type: Object, | ||
default: {} | ||
} | ||
} |
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.
Just wanted to mention: if we don't need any special default settings then we can shorten this to:
static values = { | |
options: { | |
type: Object, | |
default: {} | |
} | |
} | |
static values = { | |
options: Object | |
} |
In Slack I assumed we'd need some, but may be not? Then Slim Select is even more perfect for us 😁
// though I guess we'll need some settings for translations
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.
Yeah, we do. I still have to research the potential configuration options
Description
Since introducing choices to the backend, we have had to modify it a lot due to its nature of wrapping i 8000 tself, triggering a disconnect and then a reconnect in the DOM.
There is nothing against choices, and I consider it the most accessible fit for our case, but circumventing around a "design choice" (Choices-js/Choices#18 (comment)) creates more of a headache than simply replacing it with an easier-to-implement solution.
Workarounds we already tried
After creating the initial PRs (#4363 and #7824) and trying to use it with Hotwired Turbo and Stimulus, we had quite a few fixes, making a usually normal Stimulus controller more complex than needed:
However, as the journey continues, the move to the shadow root already broke the attached onchange listener on, e.g., the content element selection, sending the old value instead of the new one, even tho the change event was the value change from the shadow root.
We don't know what other bugs may occur, but I consider this a sunk-cost fallacy trying to dance around a design decision from 2016 simply because of being a "beauty of wrapping the original element".
Solution
As (not briefly) discussed, it would be easier to simply replace it so here we go.
ToDo's