-
-
Notifications
You must be signed in to change notification settings - Fork 105
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
enable autocomplete to accept dropdown options #60
enable autocomplete to accept dropdown options #60
Conversation
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.
LGTM
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.
Looks fine from me. 👍
On the other side, I just found on the "select" documentation, there's additional documentation on Properties section. But... The name and description IMO doesn't make sense... More like... it tries to use existing variable but for different purpose? 😕
materialize/jade/page-contents/select_content.html
Lines 284 to 288 in 88fa44a
<tr> | |
<td>dropdownOptions</td> | |
<td>Element</td> | |
<td>Dropdown UL element.</td> | |
</tr> |
@Smankusors - I didn't notice that it. I used the select component as a reference when making this change and did notice that "dropdownOptions" is used for different purposes and is only differentiated by location / scope. Below is where the options that are passed to the dropdown component are used. The dropdownOptions in the documentation you linked are a property of the select component itself, not it's options, if that makes sense. Line 243 in 88fa44a
In any case, it probably would be good to rename one of those "dropdownOptions" on the select component to clear up confusion. Maybe something for a new release since that would probably be a breaking change? |
👍 |
This confused me for a long time but actually, they named it perfectly - This would be a good feature add, along with allowing extra data to be passed to autocomplete - two use cases I come up against fairly frequently in the Gitter channel. |
that would be a documentation improvement, and IMO should be a separate PR for that.
hmm makes sense. aside from that, is this PR okay to merge? |
Not sure who you are asking but, from my perspective, this PR is ok to be merged. |
Thanks for the reviews, I'll merge it now. |
Proposed changes
document.body
.Types of changes
Checklist: