zestia / ember-select-box

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

somehow yield active option? #44

Closed miguelcobain closed 2 years ago

miguelcobain commented 2 years ago

Just wondering if it would be easy to yield on the api the currently active option to conditionally set a class. That way we could do something like:

{{#each @options as |o|}}
  <sb.Option class={{if (eq sb.activeOption o) "bg-primary-200"}} @value={{o}}>

another alternative could be (inspired by ember's built-in <LinkTo/> component):

{{#each @options as |o|}}
  <sb.Option @activeClass="bg-primary-200" @value={{o}}>
amk221 commented 2 years ago

👋

Would this work as a compromise?

<sb.Option as |option|>
  <span class={{if option.isActive "bg-primary-200"}}>

  </span>
</sb.Option>

Don't forget you can already style the active option like this:

.select-box__option[aria-current='true'] {
  @include bg-primary-200;
}
miguelcobain commented 2 years ago

Those alternatives did occurr to me. "Problem" with 2 is that I'm using tailwind. Not a real problem exactly, but we generally just add classes with it and never write css.

Option 1 is a good workaround, but of course doesn't style the same element. Perhaps I'll just use sb.Option as a wrapper element, and the styled version goes inside it. Perhaps a mix of both.

They're both great solutions. I was just wondering if there was an easy way to support another approach that was more natural to my use case/intuition.

I saw some other issue on this repo that talked about support a similar @theme argument as ember-yeti-table supports. That would be another possibility.

In any case, I can see good reasons for both adding this and not adding. If maintainers decide to add, I'm willing to make a PR. If not, I totally understand, and will use one of the existing ways.

Btw, this addon is perhaps one of the most flexible ones I've seen for a select box. Don't know how I wasn't aware of it. Great job!

amk221 commented 2 years ago

Thank you, I understand.

I'm loath to add activeOption, because it would expose the component instance itself, and would also set expectations that selectedOption would exist too.

I'm going to close the issue I'm afraid, good luck :)

miguelcobain commented 2 years ago

Just to be clear, when I said activeOption I meant activeValue. We already have a "selectedValue" which is just value.

amk221 commented 2 years ago

Ok, I think that doesn't really make sense in terms of the internal architecture

By which I mean, value is not selectedValue. Rather, each option computes whether or not they are selected using derived state based on value. And each option computes whether or not it is active based on the index of itself amongst the other options. So an option is active based on order, not value.