Skip to content

feat(select): add compareWith Input for object value comparison #11965

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

Merged
merged 5 commits into from
Jun 15, 2017
Merged

feat(select): add compareWith Input for object value comparison #11965

merged 5 commits into from
Jun 15, 2017

Conversation

zakton5
Copy link
Contributor

@zakton5 zakton5 commented Jun 7, 2017

Short description of what this resolves:

Selects that use objects as values can end up with different object identities than the currently selected object, resulting in blank selects. More details provided in Angular link below.

Changes proposed in this pull request:

Leverage the same principle used in Angular's compareWith

Ionic Version: 3.x

Fixes: #6625

Sorry about the multiple commits

@zakton5 zakton5 changed the title Select compare with feat(select): add compareWith Input for object value comparison Jun 7, 2017
Copy link
Contributor

@manucorporat manucorporat left a comment

Choose a reason for hiding this comment

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

});
} else {
option.selected = this.getValues().some(selectValue => {
return isCheckedProperty(selectValue, option.value);
Copy link
Contributor

Choose a reason for hiding this comment

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

what is isCheckedProperty is the default, compare function? this way we don't have to duplicate code. does it make sense?

Copy link
Contributor Author

@zakton5 zakton5 left a comment

Choose a reason for hiding this comment

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

Is this what you meant? @manucorporat

this._compareWith = fn;
}

private _compareWith: (o1: any, o2: any) => boolean = isCheckedProperty;
Copy link
Contributor

Choose a reason for hiding this comment

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

can you move private _compareWith: (o1: any, o2: any) => boolean = isCheckedProperty; just below _text: string = ''; (line 180)? also, drop the private property.

Copy link
Contributor

Choose a reason for hiding this comment

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

@manucorporat
Copy link
Contributor

Is this what you meant? @manucorporat

yeah! now looks beautiful

Copy link
Contributor Author

@zakton5 zakton5 left a comment

Choose a reason for hiding this comment

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

@manucorporat How's this?

@manucorporat
Copy link
Contributor

@zakton5 looks perfect now! but I prefer to wait until next version (3.5). 3.4 is scheduled for next wednesday.

@zakton5
Copy link
Contributor Author

zakton5 commented Jun 9, 2017

Alright, thanks for your help!

@manucorporat
Copy link
Contributor

Thanks a lot for the PR!!

@zakton5
Copy link
Contributor Author

zakton5 commented Jun 20, 2017

@manucorporat No problem! Thanks for your help

@zakton5 zakton5 deleted the select-compareWith branch June 20, 2017 18:29
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.

3 participants