-
Notifications
You must be signed in to change notification settings - Fork 11
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
Use named blocks instead of displayKey #36
base: main
Are you sure you want to change the base?
Conversation
Hmmm.. Looks like tests are failing in older ember versions which don't have named block support. |
Yep that would make sense. Perhaps it's best to adjust the versions ember try uses or skip ember try altogether for now until more versions support it. With this upgrade we'll likely be bumping this package as a major version change anyway, declaring we're only supporting Ember versions after the one with named blocks. Over time ember try can check more versions again when more Ember versions moving forward have support. |
Tests passed |
Great! I'd love to give it a try a little later. Thanks so much for your work on this! |
Just wanted to say sorry for not getting to this yet. It's been weird (but good) in my personal world lately and I haven't been in open source much since Ember Conf. I'll get to this when I can though~ |
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 works great! I've left a few comments of changes I'd like to see before merging this in, if any of them seem confusing or otherwise there isn't time for them I'm happy to make the changes :)
Thanks again for your hard work on getting this PR up.
@valueKey="val" | ||
@displayKey="description" /> | ||
@valueKey="val"> | ||
<:option as |optionValue| >{{optionValue.description}}</:option> |
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.
With this new style, it seems more like we are yielding the entire option and not just the option value. So perhaps this is more appropriate.
<:option as |optionValue| >{{optionValue.description}}</:option> | |
<:option as |currentOption| >{{currentOption.description}}</:option> |
@@ -153,8 +153,9 @@ module('Integration | Component | select-light', function(hooks) { | |||
<SelectLight | |||
@options={{this.options}} | |||
@value={{this.value}} | |||
@valueKey="val" | |||
@displayKey="description" /> | |||
@valueKey="val"> |
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 test no longer passes in a @displayKey
so the test description could be adjusted to reflect this. Something like should render a named option block with a customized value key when passed array of objects
perhaps.
It could then be good to add another test that uses a custom passed in @displayKey
@@ -15,7 +15,11 @@ | |||
<option | |||
value={{get optionValue this.valueKey}} | |||
selected={{is-equal (get optionValue this.valueKey) @value}}> | |||
{{get optionValue this.displayKey}} | |||
{{#if @valueKey}} |
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 @valueKey}} | |
{{#if (has-block "option")}} |
Close! I think this could follow the following pattern rather than relying on the seemingly unrelated (but passing the test) @valueKey
https://github.com/emberjs/ember.js/pull/19318/files#diff-08e773e2dfe82a277c4837362632f2e6d5d2d18137feaa80456e7d0537ff37d8R185
Closes #32