-
Notifications
You must be signed in to change notification settings - Fork 6
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
Create autocomplete component #102
Create autocomplete component #102
Conversation
type: String, | ||
default: '', | ||
}, | ||
optionTextFn: { |
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.
where is this used?
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.
It was not, deleted. Thanks!
type: Function, | ||
default: null, | ||
}, | ||
propText: { |
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.
would be better to name it propId
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.
although it matches ui-select interface :-)
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.
If our options are passed as array of objects, this prop is used for deciding which key should be used to display the text so in my opinion makes sense to be named like this. The default 'id' value may be replaced by 'name', 'label' etc, then calling it propId
could be more confusing
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.
propKey
then :-)
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.
But there is another one called propValue
having the same idea, where we decide which key
from same object will be used as a value
of the selected element. Maybe the naming is not perfect, but it captures ok what is this prop used for, plus anyway as you mentioned it matches the 'select' and I prefer to keep consistency :)
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.
okay, you are right I think it should be changed in both places, maybe later. because it's a key, it should be unique.
30dbad1
to
33b2ea9
Compare
Quality Gate failedFailed conditions |
UI reviewed with Max T.