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(radio-group): add compareWith property #28452

Merged
merged 10 commits into from
Nov 9, 2023
Merged

feat(radio-group): add compareWith property #28452

merged 10 commits into from
Nov 9, 2023

Conversation

mapsandapps
Copy link
Contributor

@mapsandapps mapsandapps commented Nov 1, 2023

Issue number: resolves #18943


What is the current behavior?

Radio Group only allows for a strict equality check between the value and the options.

What is the new behavior?

  • Radio Group has a new optional property compareWith which can be a function or string.
  • If compareWith is a string, the Radio Group will compare the value of that key.
  • If compareWith is a function, the Radio Group will use that function to determine the selected option.
  • Refactored Select a bit to reuse functionality between it and Radio Group.

Does this introduce a breaking change?

  • Yes
  • No

Other information

Docs PR is incoming.

@github-actions github-actions bot added the package: core @ionic/core package label Nov 1, 2023
@github-actions github-actions bot added package: angular @ionic/angular package package: vue @ionic/vue package labels Nov 1, 2023
@mapsandapps mapsandapps marked this pull request as ready for review November 2, 2023 22:09
Copy link
Contributor

@liamdebeasi liamdebeasi left a comment

Choose a reason for hiding this comment

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

Functionality looks great. My comments are focused around the tests.

core/src/components/radio-group/radio-group.tsx Outdated Show resolved Hide resolved
core/src/components/select/select.tsx Outdated Show resolved Hide resolved
@@ -0,0 +1,37 @@
type CompareFn = (currentValue: any, compareValue: any) => boolean;

export const compareOptions = (
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we add a JSDoc for compareOptions too like we did for isOptionSelected?

@liamdebeasi liamdebeasi changed the title feat(ion-radio-group): add compareWithProperty feat(radio-group): add compareWithProperty Nov 3, 2023
@mapsandapps mapsandapps changed the title feat(radio-group): add compareWithProperty feat(radio-group): add compareWith property Nov 3, 2023
Copy link
Contributor

@liamdebeasi liamdebeasi left a comment

Choose a reason for hiding this comment

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

One naming change, but otherwise looks good. Great job!

core/src/utils/forms/compare-with-utils.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@thetaPC thetaPC left a comment

Choose a reason for hiding this comment

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

LGTM

@mapsandapps mapsandapps merged commit 0ae327f into feature-7.6 Nov 9, 2023
43 checks passed
@mapsandapps mapsandapps deleted the FW-728 branch November 9, 2023 15:21
@liamdebeasi
Copy link
Contributor

@mapsandapps Don't forget to close #18943 too. GitHub doesn't autoclose linked issues when merging into the non-primary branch.

@mapsandapps
Copy link
Contributor Author

@liamdebeasi thanks for the reminder!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
package: angular @ionic/angular package package: core @ionic/core package package: vue @ionic/vue package
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants