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

feat(select): add compareWith property #17358

Merged
merged 19 commits into from Mar 1, 2019

Conversation

4 participants
@zakton5
Copy link
Contributor

commented Feb 1, 2019

Short description of what this resolves:

Using objects as values was only supported by comparing object equality (===) but not by content. This PR adds a compareWith property that accepts a property name (ie id) or a function of the shape (o1, o2) => boolean that is used to determine equality.

Changes proposed in this pull request:

  • Add compareWith prop
  • Move internal option comparison logic to its own function: compareOptions

Ionic Version: 4.0.0

Fixes: #14598

zakton5 and others added some commits Jan 22, 2019

Merge pull request #1 from ionic-team/master
update to latest 1-22-19
Merge pull request #2 from ionic-team/master
Update master 2-1-19
Zach Keeton

@zakton5 zakton5 changed the title Feat select track by feat(select): add compareWith property Feb 1, 2019

@brandyscarney brandyscarney added this to In progress 🤺 in Ionic Core via automation Feb 1, 2019

@brandyscarney brandyscarney moved this from In progress 🤺 to Needs review 🤔 in Ionic Core Feb 8, 2019

@remisture

This comment has been minimized.

Copy link

commented Feb 15, 2019

ETA for this?

@zakton5

This comment has been minimized.

Copy link
Contributor Author

commented Feb 15, 2019

@remisture 🤷‍♂️ Whenever the Ionic team reviews it. Hard to say..

zakton5 and others added some commits Feb 15, 2019

@brandyscarney

This comment has been minimized.

Copy link
Member

commented Feb 22, 2019

Thank you for the PR!

I have this added to review for 4.1.0. We were working out the process for how we would move forward with features so we haven't been adding them, but we are now! Although I'd almost consider this a fix as it's a regression. I have added myself and some other coworkers to review this.

My only request at first glance is could you please move the test out of the preview/ folder - maybe we could change the basic test? We're planning to remove the preview folders that's why I ask. 🙂

@zakton5

This comment has been minimized.

Copy link
Contributor Author

commented Feb 27, 2019

Will do @brandyscarney!

zakton5 and others added some commits Feb 27, 2019

@liamdebeasi
Copy link
Member

left a comment

This is looking really good so far!

Show resolved Hide resolved core/src/components/select/select.tsx Outdated

liamdebeasi and others added some commits Feb 28, 2019

@liamdebeasi liamdebeasi merged commit 69ecebb into ionic-team:master Mar 1, 2019

1 check passed

build Workflow: build
Details

Ionic Core automation moved this from Needs review 🤔 to Done 🎉 Mar 1, 2019

@brandyscarney

This comment has been minimized.

Copy link
Member

commented Mar 1, 2019

Thank you @zakton5 !

@zakton5 zakton5 deleted the zakton5:feat-select-track-by branch Mar 3, 2019

franmosteiro pushed a commit to laRivolta/daChecklist that referenced this pull request Mar 9, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.