add ability to have a selectable prompt from the select dropdown #35
Conversation
There was a problem hiding this comment.
Hi @steverhoades, thanks for your contribution!
I added some feedback to the code. Two comments adress the wrapping of one-way-inputs, which I'm generally not very happy with at the moment because we need to replicate every single parameter. Going forward, I'd like to explore using https://github.com/ciena-blueplanet/ember-spread which might simplify this drastically. But that's something we'll look at in the future; if you can provide same feedback or code changes to my comments I'm happy to merge this! 😄
| optionValuePath = optionValuePath | ||
| optionTargetPath = optionTargetPath | ||
| name = name | ||
| class = "form-control" |
There was a problem hiding this comment.
I think rather than changing this line you should add your specific CSS class to the config (see README) - or am I missing something here?
There was a problem hiding this comment.
Woops. Yes, the class names should be fixed.
| class = "form-control" | ||
| update = (action "update") | ||
| focusOut = (action "setDirty") | ||
| prompt = (if includeBlank (or includeBlank null)) |
There was a problem hiding this comment.
prompt is currently implemented as an alias for includeBlank (which is not ideal, see DavyJonesLocker/ember-one-way-controls#152). Also, i'd prefer this addon to involve as less "magic" as possible when it comes to wrapping one-way-inputs. Since supporting both prompt and includeBlank seems to be impossible at the moment, i'd suggest just passing
prompt = prompt
There was a problem hiding this comment.
I just realized that we were supporting includeBlank already. Since prompt and includeBlank are functionally equivalent, it might make most sense to just keep includeBlank = includeBlank, and add a little comment ("We're not supporting prompt, please just use includeBlank instead") to the README. As soon as the issue in one-way-controls is fixed, we could add prompt.
In case I'm missing something here it might make sense if you could shed some more light on your concrete use-case for this!
There was a problem hiding this comment.
I'll change it back.
| prompt = (if includeBlank (or includeBlank null)) | ||
| disabled = disabled | ||
| multiple = multiple | ||
| promptIsSelectable = (if promptIsSelectable (or promptIsSelectable false)) |
There was a problem hiding this comment.
Same here, I'd prefer promptIsSelectable = promptIsSelectable
There was a problem hiding this comment.
The reason I added promptIsSelectable in this way was to maintain backwards compatibility with any existing implementations, but I'm happy to change it.
There was a problem hiding this comment.
Isn't promptIsSelectable = promptIsSelectable also backwards compatible? false is the default and we didn't support this option before.
There was a problem hiding this comment.
That makes sense.
|
@czosel Provided a pull request to should fix the issues raised with your comments. Let me know if you have any more feedback. Cheers! |
|
Hi @steverhoades, since your PR was aaalmost ready to merge and starting to get a little dusty, i went ahead and added the last bits in #40. Thanks again for your contribution! |
|
@czosel now that this issue has been fixed, would you be adding support for prompt and promptIsSelectable to Ember Validated Form? It would be greatly appreciated. |
|
@sihuang Which issue do you mean? It seems to me that the related issue in one-way-components still exists. Feel free to open another issue here if i misunderstood 😉 |
Allow for a selectable blank entry.
This allows for a "select" to have a prompt ie> "Select a widget" and a validation of validatePresence(true).
I kept includeBlank to keep consistent with the documentation.