Skip to content

Conversation

@Gimly
Copy link

@Gimly Gimly commented Oct 2, 2017

This function allows to add a comparison function for the option used as a value. It is called when searching for the option in the options array. If it is not defined, it defaults to a standard === comparison.

I haven't fully tested yet because I'm not quite sure that this is going in the right direction. If it seems to be correct, I'll update and add maybe an example.

Before submitting a pull request, please make sure you have at least performed the following:

  • read and followed the CONTRIBUTING.md guide.
  • [] linted, built and tested the changes locally.
  • added/updated any applicable API documentation.
  • added/updated any applicable demos.

Gimly added 2 commits October 2, 2017 22:57
This function allows to add a comparison function for the option used as a value. It is called when searching for the option in the options array. If it is not defined, it defaults to a standard === comparison.
Copy link
Owner

@edcarroll edcarroll left a comment

Choose a reason for hiding this comment

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

Thank you for opening this, I'm sorry it's taken so long to be looked at. 2 points and then we're good to go.

"This must be defined as an arrow function in your class."
},
{
name: "compareWith",
Copy link
Owner

Choose a reason for hiding this comment

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

Could you name this function optionsCompare please to get it inline with optionsLookup?

public abstract selectOption(option:T):void;

protected findOption(options:T[], value:U):T | undefined {
if (this._compareWith) {
Copy link
Owner

Choose a reason for hiding this comment

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

I think rather than checking to see if the function has been assigned, it would be cleaner to initialise this property in the constructor with (a, b) => a === b , then you don't need this check.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants