-
Notifications
You must be signed in to change notification settings - Fork 0
IBX-10850: AltRadio List Field #64
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
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR implements an AltRadio List Field component, introducing new functionality for rendering and managing lists of alternative radio button inputs with their associated TypeScript behavior.
Key Changes:
- Added backend PHP component class and Twig template for AltRadio list fields
- Implemented TypeScript class to manage AltRadio list field interactions
- Updated initialization code to support the new component
Reviewed Changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
src/lib/Twig/Components/AltRadio/ListField.php |
Defines the PHP component class for AltRadio list fields with configuration |
src/bundle/Resources/views/themes/standard/design_system/components/alt_radio/list_field.html.twig |
Twig template for rendering the AltRadio list field component |
src/bundle/Resources/views/themes/standard/design_system/components/alt_radio/input.html.twig |
Updated radio input template to support custom initialization and name attribute |
src/bundle/Resources/public/ts/init_components.ts |
Updated component initialization to include AltRadio list fields |
src/bundle/Resources/public/ts/components/alt_radio/index.ts |
Exports the new AltRadiosListField class |
src/bundle/Resources/public/ts/components/alt_radio/alt_radios_list_field.ts |
TypeScript implementation for managing AltRadio list field behavior |
src/bundle/Resources/public/ts/components/alt_radio/alt_radio_input.ts |
Enhanced radio input class to support list field integration |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
|
|
||
| setError(value: boolean) { | ||
| this._tileElement.classList.toggle('ids-alt-radio__tile--error', value); | ||
| this.tileElement.classList.toggle('ids-alt-radio_tile--error', value); |
Copilot
AI
Oct 23, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Corrected class name from 'ids-alt-radio_tile--error' to 'ids-alt-radio__tile--error' for BEM naming consistency (should use double underscore).
| this.tileElement.classList.toggle('ids-alt-radio_tile--error', value); | |
| this.tileElement.classList.toggle('ids-alt-radio__tile--error', value); |
2cba38f to
f6fd6d6
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (!(event.target instanceof HTMLDivElement)) { | ||
| return; | ||
| } | ||
|
|
Copilot
AI
Oct 30, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The type check is too restrictive. The event target could be any element within the tile (like text nodes or other HTML elements), not just HTMLDivElement. This will prevent clicks on child elements from working properly.
| if (!(event.target instanceof HTMLDivElement)) { | |
| return; | |
| } | |
| // Allow clicks from any element within the tile, not just HTMLDivElement |
| detail: itemValue, | ||
| }); | ||
|
|
||
| const currentValueInstance = this.itemsMap.get(this.value ?? ''); |
Copilot
AI
Oct 30, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using empty string as fallback for undefined value could cause issues if there's an actual item with an empty string ID. Consider using a different approach or ensuring empty string IDs are not valid.
| const currentValueInstance = this.itemsMap.get(this.value ?? ''); | |
| let currentValueInstance: AltRadioInput | undefined; | |
| if (this.value !== undefined) { | |
| currentValueInstance = this.itemsMap.get(this.value); | |
| } |
| currentValueInstance.toggleChecked(false); | ||
| } | ||
|
|
||
| const nextValueInstance = this.itemsMap.get(itemValue); |
Copilot
AI
Oct 30, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using empty string as fallback for undefined value could cause issues if there's an actual item with an empty string ID. Consider using a different approach or ensuring empty string IDs are not valid.
|
@GrabowskiM / @mikadamczyk Rebase is needed here |
877c02c to
12a381f
Compare
Related PRs:
Description:
For QA:
Documentation: