zestia / ember-select-box

:capital_abcd: A faux select box for Ember apps
MIT License
64 stars 14 forks source link

Option's value as string instead of object? #36

Closed ChrizzDF closed 3 years ago

ChrizzDF commented 3 years ago

Hi there! I came across this nice package and it seems like it's exactly what I was looking for - thx!

My issue is, that I I'm not able to use a native select with native <option>s to hold an object as a value. For example:

<NativeSelectBox @onSelect={{fn (mut this.selectedContinent)}} as |sb|>
  {{#each @model.continents as |c|}}
    <sb.Option @value={{c}}>
      {{c.name}}
    </sb.Option>
  {{/each}}
</NativeSelectBox>

The value should hold the whole continent model but it's just the string representation of the model itself:

value="<ember-boilerplate@model:continent::ember426:AF>"

The strange thing is: I looked at your examples, inspected the example and the values of that first select do have objects as values 😳

value="[object Object]"

Do you have any idea what I'm doing wrong?

Thanks in advance! 🙏

amk221 commented 3 years ago

Hi 👋

So the way it works is the value HTML attribute is not really used.

Each option, is an option component, and the @value is stored on that component, be it a primitive like a number, or a non-primitive like a complex object.

When the user interacts with the component, onchange will fire, as you'd expect.

The addon then looks at which option elements have been selected, and fires onSelect with the @value from the component, not the value from the value attribute.

There is a test to check this scenario works, here: https://github.com/zestia/ember-select-box/blob/main/tests/integration/components/native-select-box/select-box-test.js#L131-L156

Can you try adding some more debugging? I can't see anything obviously wrong in your example

ChrizzDF commented 3 years ago

Thanks for the quick reply!

If I use the mut helper on the @onSelect, It's always taking what's inside the value of the <option> which in my case is always a string.

If I use an action (@onSelect={{this.setSelectedContinent}}), I'm able to receive the whole object inside the action handler.

But since I'm lazy, I would've preferred just using the mut helper but I will read through the Ember Docs because it's not a problem of your nice addon :)

Maybe you find some minutes someday to also try to find out if/how you could use the mut helper like

@onSelect={{action (mut this.selectedContinent)}}

and mutating the whole object, not just a prop value like the name.

Definitely thx for the support, but I think you can surely close this issue!

amk221 commented 3 years ago

That's strange, it works for me:

https://github.com/zestia/ember-select-box/commit/b8ae2aeb43ad88909d76f533fb97f972843865db

🤔

amk221 commented 3 years ago

If you can push up a reproduction repo, I’ll take a look

ChrizzDF commented 3 years ago

Hi, I'm sorry for the delay...

I set up a repo: https://github.com/ChrizzDF/ember-native-select

I noticed that the option is not bound as an object when you use an Ember model. If using POJOs, it's all fine.

amk221 commented 3 years ago

Thanks for the repo!

You can factor-out ember-select-box from the equation.

Here is a Twiddle: https://ember-twiddle.com/7f01a7a2890476b2ccef43b59c5eab63?openFiles=templates.application%5C.hbs%2C

As far as I can see there is no issue?

ChrizzDF commented 3 years ago

Thanks for checking this!!!

I'm a bit confused because

<NativeSelectBox
  @onSelect={{action (mut this.selectedContinent)}}
  class="appearance-none h-10 bg-gray-50 px-3 w-full inline-flex border rounded-lg focus:outline-none focus:ring-2 focus:border-blue-500"
  as |sb|
>
  <option value="All">All</option>
  {{#each @model.continents as |c|}}
    <sb.Option @value={{c}}>
      {{c.name}}
    </sb.Option>
  {{/each}}
</NativeSelectBox>

is now doing exactly what I wanted to have. Even though the value of the option holds the model representation as a string, it will be set as the model instance!

Thanks again and keep up the good work! :)

amk221 commented 3 years ago

Yep, it's redundant really, perhaps I wasn't clear enough here