Skip to content

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

Merged
mapsandapps merged 10 commits into
feature-7.6from
FW-728
Nov 9, 2023
Merged

feat(radio-group): add compareWith property#28452
mapsandapps merged 10 commits into
feature-7.6from
FW-728

Conversation

@mapsandapps

@mapsandappsmapsandapps commented Nov 1, 2023•
edited by liamdebeasi
Loading

Copy link
Copy Markdown
Contributor

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.

mapsandappsand others added 4 commits November 1, 2023 15:36
@mapsandapps@sean-perkins
feat(radio-group): add compareWith property
2425bee
Co-authored-by: Sean Perkins <sean-perkins@users.noreply.github.com>
@mapsandapps
Fix value in tests
5187f7c
@mapsandapps
Add tests
45e4e14
@mapsandapps
Add test
52b99e6
@github-actionsgithub-actionsBot added the package: core @ionic/core package label Nov 1, 2023
@github-actionsgithub-actionsBot 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
liamdebeasi
liamdebeasi suggested changes Nov 3, 2023
View reviewed changes

@liamdebeasiliamdebeasi left a comment

Copy link
Copy Markdown
Contributor

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.

Comment threadcore/src/components/radio-group/test/compare-with/radio-group.e2e.ts Outdated
liamdebeasi
liamdebeasi reviewed Nov 3, 2023
View reviewed changes
Comment threadcore/src/components/radio-group/radio-group.tsx Outdated
Comment threadcore/src/components/select/select.tsx Outdated
Comment threadcore/src/utils/forms/compare-with-utils.ts
@@ -0,0 +1,37 @@
type CompareFn = (currentValue: any, compareValue: any) => boolean;

export const compareOptions = (

@liamdebeasiliamdebeasiNov 3, 2023

Copy link
Copy Markdown
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?

@liamdebeasiliamdebeasi changed the title feat(ion-radio-group): add compareWithPropertyfeat(radio-group): add compareWithPropertyNov 3, 2023
@mapsandappsmapsandapps changed the title feat(radio-group): add compareWithPropertyfeat(radio-group): add compareWith propertyNov 3, 2023
liamdebeasi
liamdebeasi approved these changes Nov 6, 2023
View reviewed changes

@liamdebeasiliamdebeasi left a comment

Copy link
Copy Markdown
Contributor

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!

Comment threadcore/src/utils/forms/compare-with-utils.ts Outdated
@mapsandapps@liamdebeasi
Update core/src/utils/forms/compare-with-utils.ts
cdb58d3
Co-authored-by: Liam DeBeasi <liamdebeasi@users.noreply.github.com>
@mapsandappsmapsandapps mentioned this pull request Nov 8, 2023
Merged
thetaPC
thetaPC approved these changes Nov 8, 2023
View reviewed changes

@thetaPCthetaPC left a comment

Copy link
Copy Markdown
Contributor

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.6Nov 9, 2023
@mapsandapps
mapsandapps deleted the FW-728 branch November 9, 2023 15:21
@liamdebeasi

liamdebeasi commented Nov 9, 2023

Copy link
Copy Markdown
Contributor

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

@mapsandapps

mapsandapps commented Nov 9, 2023

Copy link
Copy Markdown
ContributorAuthor

@liamdebeasi thanks for the reminder!

dietergeerts
dietergeerts reviewed Jul 16, 2025
View reviewed changes
Comment threadcore/src/utils/forms/compare-with-utils.ts
if (typeof compareWith === 'function') {
return compareWith(currentValue, compareValue);
} else if (typeof compareWith === 'string') {
return currentValue[compareWith] === compareValue[compareWith];

@dietergeertsdietergeertsJul 16, 2025•
edited
Loading

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

currentValue can be null with Angular forms, so this throws errors when using the property string variant :(. This is still an issue in the latest 8.6.4 version.

@dietergeertsdietergeertsJul 16, 2025

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

While I was further using this component, I also see the issue that deselection doesn't work initially. This is because this custom compare isn't applied in the logic to see if a value should be deselected.

@liamdebeasiliamdebeasiJul 16, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@dietergeerts Can you file an issue and include a reproduction? The team doesn't typically monitor closed PRs.

@dietergeertsdietergeertsJul 18, 2025

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Created with reproduction: #30560

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

Reviewers

@thetaPCthetaPCthetaPC approved these changes
@averyrousseauaveryrousseauAwaiting requested review from averyrousseau
@sean-perkinssean-perkinsAwaiting requested review from sean-perkins
+2 more reviewers
@liamdebeasiliamdebeasiliamdebeasi approved these changes
@dietergeertsdietergeertsdietergeerts left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

package: angular@ionic/angular packagepackage: core@ionic/core packagepackage: vue@ionic/vue package

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@mapsandapps@liamdebeasi@dietergeerts@thetaPC