Skip to content

Conversation

@polina-c
Copy link
Collaborator

@polina-c polina-c commented Oct 26, 2025

This limitation does not work: #448

Screenshot 2025-10-26 at 12 29 14 PM

@polina-c polina-c requested a review from gspencergoog October 26, 2025 19:26
Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request adds an example for using maxAllowedSelections: 1 in the MultipleChoice widget. My review has identified two main issues. First, the new example is added incorrectly; it should be a separate entry in the exampleData list. Second, and more critically, the maxAllowedSelections feature is not actually implemented in the widget's logic, nor is it tested. This makes the example misleading. I've provided a suggestion to fix the example's structure and have detailed the missing implementation logic and testing requirements in a separate comment.

"selections": {
"path": "/mySelections"
},
"maxAllowedSelections": 1,
Copy link
Contributor

Choose a reason for hiding this comment

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

critical

The maxAllowedSelections property is a great addition, but its logic doesn't seem to be implemented in the MultipleChoice widget builder.

Specifically:

  • The onChanged handler (lines 90-105) adds/removes items from the selections list without respecting maxAllowedSelections. When an item is selected, it should check if the limit is reached. If maxAllowedSelections is 1, selecting a new item should deselect any previously selected item.
  • When maxAllowedSelections is 1, it would be more intuitive to render RadioListTile widgets instead of CheckboxListTile to visually indicate that only one option can be selected.
  • Additionally, there are no tests for the maxAllowedSelections behavior. A test case should be added to verify the correct behavior when maxAllowedSelections is set, especially for the single-selection case.

Since this PR adds an example for a feature that is not yet implemented, it could be misleading. I'd recommend implementing this logic and adding tests as part of this PR, or removing this example for now.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

true: #448

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.

1 participant