Skip to content
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.

Sign up for GitHub

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

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Use named blocks instead of displayKey #36

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
8e6d188
Try to get namedblocks working with a test
colenso Mar 29, 2021
7152a67
Only use named blocks when @valueKey is supplied
colenso Mar 29, 2021
81dd614
Fix test
colenso Mar 29, 2021
482c6df
Update ember-try to 3.25 and above
colenso Mar 29, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion addon/components/select-light.hbs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,11 @@
<option
value={{get optionValue this.valueKey}}
selected={{is-equal (get optionValue this.valueKey) @value}}>
{{get optionValue this.displayKey}}
{{#if @valueKey}}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
{{#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

{{yield optionValue to="option"}}
{{else}}
{{get optionValue this.displayKey}}
{{/if}}
</option>
{{/each}}
{{else}}
Expand Down
1 change: 0 additions & 1 deletion addon/components/select-light.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,6 @@ export default class extends Component {
get hasDetailedOptions() {
return ![ // Returns a boolean if all data is available for a { label: foo, value: bar } style list of options
this.args.options?.[0][this.valueKey],
this.args.options?.[0][this.displayKey],
].some(isNone);
}
}
12 changes: 2 additions & 10 deletions config/ember-try.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,18 +6,10 @@ module.exports = async function () {
return {
scenarios: [
{
name: 'ember-lts-3.16',
name: 'ember-lts-3.25',
npm: {
devDependencies: {
'ember-source': '~3.16.0',
},
},
},
{
name: 'ember-lts-3.20',
npm: {
devDependencies: {
'ember-source': '~3.20.5',
'ember-source': '~3.25.0',
},
},
},
Expand Down
5 changes: 3 additions & 2 deletions tests/integration/components/select-light-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -153,8 +153,9 @@ module('Integration | Component | select-light', function(hooks) {
<SelectLight
@options={{this.options}}
@value={{this.value}}
@valueKey="val"
@displayKey="description" />
@valueKey="val">
Copy link

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

<:option as |optionValue| >{{optionValue.description}}</:option>
Copy link

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.

Suggested change
<:option as |optionValue| >{{optionValue.description}}</:option>
<:option as |currentOption| >{{currentOption.description}}</:option>

</SelectLight>
`);

assert.dom('select option').hasAttribute('value', options[0].val);
Expand Down