-
Notifications
You must be signed in to change notification settings - Fork 0
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
[RFC][UN-14114] Add groups to uni select #260
Conversation
5b2bb7c
to
2cc8435
Compare
2cc8435
to
5be7d81
Compare
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.
After already talking with @carantunes, I don't think that this is not the best solution for this problem. I think that it's not good to be able to have two separate APIs for the same component -- groups
vs. options
.
One of the problems is the following: what will happen when someone invokes {{uni-select groups=groups options=options}}
?
Also, this solution doesn't allow to create a uni-select
with groups and regular options under the same <select>
.
My suggestion, and the one I believe is the most correct solution, is having an optional field inside the option object passed to the uni-select component:
const selectOptions: [{
label: ‘this is a group’,
options: [] // If it has options, it'll behave as a group
},
{
label: 'this is a regular option'
}
]
and invoking the component as we already do:
{{uni-select options=selectOptions}}
This will lead to:
- Not having possible conflict situations: groups and options parameters both being passed
- Keeping the same API: when we want to add groups to already existent uni-select, it's adding the
options
array - Allowing
optgroup
s andoption
s to co-exist
addon/components/uni-select.js
Outdated
placeholder: null, | ||
useAlias: false, | ||
aliasValue: null, | ||
groups: [], | ||
|
||
hasGroups: computed('groups', function() { |
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.
You can use computed.gt('groups.length', 0)
I don't think its a good behaviour to have both options alone and groups in the same select. Its confusing to select an option without group, and not being able to select a group. Having that said and as this is an improvement to the code and as there's no other select with groups in the works, i suggest creating a ticket for the frontend guild and i can tackle it in the next sprint. |
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.
🔝
} | ||
}, | ||
|
||
actions: { | ||
changeSelected({ target }) { | ||
if (this.get('useAlias')) { | ||
this._changeAliasValue(target.value); | ||
let group = this.get('hasGroups') ? target.options[target.selectedIndex].parentNode.getAttribute('key') : null; |
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.
this has more than 120 chars?
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.
exactly 120 😆
addon/components/uni-select.js
Outdated
_changeAliasValue(key) { | ||
let option = A(this.get('options')).findBy('key', key); | ||
_changeAliasValue(key, group = null) { | ||
let options = this.get('options'); |
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.
let options = group ? A(this.get('groups')).findBy('key', group).options : this.get('options');
https://uniplaces.atlassian.net/browse/UN-14114