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.

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

Pass the selected value(s) as the first parameter to @change #30

Open
pzuraq opened this issue Mar 14, 2021 · 3 comments · May be fixed by #40
Open

Pass the selected value(s) as the first parameter to @change #30

pzuraq opened this issue Mar 14, 2021 · 3 comments · May be fixed by #40
Labels
enhancement New feature or request good first issue Good for newcomers help wanted Extra attention is needed

Comments

@pzuraq
Copy link

pzuraq commented Mar 14, 2021

Is your feature request related to a problem? Please describe.

When using <SelectLight>, currently you must always handle the change event yourself. This means that you must create a JavaScript event handler to update the value, like so:

<SelectLight
  @value={{this.selected}}
  @options={{array "turtle" "tortoise"}}
  @change={{this.handleChange}} 
/>
class MyComponent extends Component {
  @tracked selected = "turtle";

  handleChange = (event) => {
    this.selected = event.target.selectedOptions[0].value;
  };
}

This is a lot of boilerplate to write, and also means that you can't use a conventional solution like ember-set-helper or {{mut}} to update a value with this component.

Describe the solution you'd like

Instead of passing the event as the first value to @change, I think it would be better to pass the value of the newly selected option (or options if there are multiple):

class MyComponent extends Component {
  @tracked selected = "turtle";

  handleChange = (value, event) => {
    this.selected = value;
  };
}

This could be used conventionally with {{set}} from ember-set-helper like so:

<SelectLight
  @value={{this.selected}}
  @options={{array "turtle" "tortoise"}}
  @change={{set this "selected"}} 
/>

I think the event could still be provided as the second parameter, there are definitely cases where the user may want to do something a bit more custom and may need access to the original event.

Describe alternatives you've considered

This could possibly be provided as a separate API, @changeValue (name is definitely bikesheddable), but IMO this would be a bit confusing, since there would be multiple event handlers at that point.

@hergaiety hergaiety added good first issue Good for newcomers help wanted Extra attention is needed enhancement New feature or request and removed help wanted Extra attention is needed labels Mar 19, 2021
@hergaiety hergaiety added the help wanted Extra attention is needed label Mar 29, 2021
@pankaj-awake
Copy link

I got my eyes on this, will post my thoughts after some recon on the repo.

@hergaiety
Copy link

hergaiety commented Mar 29, 2021

I got my eyes on this, will post my thoughts after some recon on the repo.

Sounds good, thanks! I'm @gaiety#0001 on Discord if you have questions on anything :)
I'll also throw @pzuraq#9034 under the bus who wrote this issue and is also a mentor for the contributors workshop as well.

@pankaj-awake
Copy link

I apologize, for the long delay on this.
Please review ⬆️ whenever the team finds time, thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request good first issue Good for newcomers help wanted Extra attention is needed
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants