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

Allow providing callback prop to execute the command #520

Open
iddan opened this issue Sep 11, 2020 · 8 comments
Open

Allow providing callback prop to execute the command #520

iddan opened this issue Sep 11, 2020 · 8 comments
Assignees

Comments

@iddan
Copy link

iddan commented Sep 11, 2020

Is your feature request related to a problem? Please describe.
My problem is the command property must be a function. Because of that, I must use this in it for the object to be testable.

Describe the solution you'd like
I suggest introducing a new prop executeCommand: (command: Command) => void which will take the command object and execute it. That way the command object can contain only data and no this need to be used.

Additional context
An example would be:
Without executeCommand:

function go() {
    history.go(this.link);
}

const props = {
  options: [
    {
      name: "Go to example",
      link: "http://example.com",
      command: go
    },
  ],
};

With executeCommand:

const props = {
  options: [
    {
      name: "Go to example",
      link: "http://example.com",
    },
  ],
  executeCommand: (command) => {
    history.go(command.link)
  }
};
@asabaylus asabaylus self-assigned this Oct 7, 2020
@asabaylus
Copy link
Owner

@iddan can you describe what you are testing? The use of this should be optional within the command functions

@iddan
Copy link
Author

iddan commented Oct 7, 2020

I'm testing the creation of the options provided to the command palette. If they could be data only I would be able to create them with a pure function which is much easier to unit test.

@asabaylus
Copy link
Owner

Have you considered passing a noop function as commands then using onSelect to execute your function?

@iddan
Copy link
Author

iddan commented Oct 8, 2020

No, I haven't. Seem like a little more complicated than the solution I proposed but it should do.
If I understood it correctly, It will require creating custom code to the onSelect and updating the loading state manually.

@asabaylus
Copy link
Owner

Yes, you'd be using onSelect much like the browser native drop down list box and matching on the returned object so that your function could be executed.

For context, I've periodically considered eliminating the inclusion of functions passed to the commands prop. It's really not required and breaks with native browser control conventions. However, I wanted a batteries included component the was easy to use.

@iddan
Copy link
Author

iddan commented Oct 8, 2020

I agree it's a simpler approach. That is why I'm suggesting a more React-ive solution for the problem that separates data from function.

@asabaylus
Copy link
Owner

@iddan I've got a few other enhancements to get to first but I'm open to a pull request

@iddan
Copy link
Author

iddan commented Oct 11, 2020

I'll try to get around it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants