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

Added compareWith input for select #310

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

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.

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.

@@ -197,6 +197,11 @@ export class SelectPage {
"Must return a <code>Promise</code>. " +
"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?

@@ -331,6 +341,9 @@ export abstract class SuiSelectBase<T, U> implements AfterContentInit, OnDestroy
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.

None yet

2 participants