Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR introduces a new Toggle Button component to the design system, consisting of both input and field implementations with corresponding Twig templates and TypeScript classes.
Key Changes:
- Added Toggle Button component with PHP backend classes for input and field handling
- Created Twig templates for rendering toggle button UI elements
- Implemented TypeScript classes for toggle button functionality and initialization
Reviewed Changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
src/lib/Twig/Components/ToggleButton/Input.php |
Defines the toggle button input component with configurable on/off labels and translation support |
src/lib/Twig/Components/ToggleButton/Field.php |
Defines the toggle button field wrapper component with input attribute handling |
src/bundle/Resources/views/themes/standard/design_system/components/toggle_button/input.html.twig |
Template for rendering the toggle button input with checkbox source and label toggling |
src/bundle/Resources/views/themes/standard/design_system/components/toggle_button/field.html.twig |
Template for rendering the toggle button field wrapper |
src/bundle/Resources/public/ts/init_components.ts |
Initializes toggle button components on page load |
src/bundle/Resources/public/ts/components/toggle_button/toggle_button_input.ts |
TypeScript class implementing toggle button input behavior |
src/bundle/Resources/public/ts/components/toggle_button/toggle_button_field.ts |
TypeScript class implementing toggle button field wrapper behavior |
src/bundle/Resources/public/ts/components/toggle_button/index.ts |
Exports toggle button TypeScript modules |
composer.json |
Adds symfony/uid dependency for unique ID generation |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
GrabowskiM
force-pushed
the
IBX-10853-toggle
branch
2 times, most recently
from
8c6c272 to
125171d
Compare
| protected updateLabel(): void { | ||
| const isChecked = this._inputElement.checked; | ||
|
|
||
| if (isChecked) { |
There was a problem hiding this comment.
WDYT?
| if (isChecked) { | |
| this.toggleLabelNode.textContent = isChecked ? this.labels.on : this.labels.off; |
| final class Field extends AbstractField | ||
| { | ||
| /** @var non-empty-string */ | ||
| public string $id; // TODO: maybe move to AbstractField? |
There was a problem hiding this comment.
I wouldn’t move the ID handling up to AbstractField. That base class is used by every field wrapper in the bundle, including list‑style components like the checkbox/radio groups, where there is no single input to bind to a label. Forcing an ID there would either require fake values or bloating components that don’t need it.
GrabowskiM
force-pushed
the
IBX-10853-toggle
branch
from
8fa4e67 to
e68d1c3
Compare
| /** | ||
| * @phpstan-type AttributeMap array<string, scalar> | ||
| */ | ||
| trait SingleInputFieldTrait |
There was a problem hiding this comment.
tbh, this seems like a little to much and to specified for a trait, is there a way to make service from it and inject it where needed?
There was a problem hiding this comment.
After internal sync with @Steveb-p, I changed the trait to the AbstractSingleInputField
konradoboza
left a comment
konradoboza
left a comment
There was a problem hiding this comment.
But @ViniTou comment might be valid - depends on how widely would this trait be used.
| ->allowedTypes('array') | ||
| ->default([]) | ||
| ->normalize(static function (Options $options, array $attributes): array { | ||
| return self::assertForbidden($attributes, ['id', 'name', 'required', 'value'], 'input'); |
There was a problem hiding this comment.
This call shows that this trait depends on something else. It can only ever be used as part of AbstractField.
…and improve structure
Related PRs:
Description:
For QA:
Documentation: