Skip to content

Conversation

@GrabowskiM
Copy link
Contributor

🎫 Issue IBX-7908

Related PRs:

Description:

For QA:

Documentation:

@GrabowskiM GrabowskiM requested review from a team, Copilot and mikadamczyk October 23, 2025 13:22
Copy link
Contributor

Copilot AI left a 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 a multivalue dropdown component (IBX-7908), allowing users to select multiple items from a dropdown list with checkboxes. The implementation extends the existing dropdown infrastructure to support multi-selection functionality.

Key changes:

  • Added DropdownMultiInput component with checkbox-based item selection
  • Extended AbstractDropdown with template property support for custom item rendering
  • Created utility function for dynamic template rendering with placeholder replacement

Reviewed Changes

Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
src/lib/Twig/Components/DropdownMulti/Input.php PHP component for multi-value dropdown with selected items handling
src/lib/Twig/Components/AbstractDropdown.php Extended base dropdown with item template property support
src/bundle/Resources/views/themes/standard/design_system/partials/base_dropdown.html.twig Updated dropdown template to support custom item content with template props
src/bundle/Resources/views/themes/standard/design_system/components/dropdown_multi/input.html.twig Twig template for multi-select dropdown with checkbox items
src/bundle/Resources/public/ts/utils/dom.ts DOM utility for creating nodes from templates with placeholder replacement
src/bundle/Resources/public/ts/partials/base_dropdown/base_dropdown.ts Updated base dropdown to support NodeList item content and removed abstract methods
src/bundle/Resources/public/ts/init_components.ts Added initialization for multi-value dropdown components
src/bundle/Resources/public/ts/components/dropdown/index.ts Added export for DropdownMultiInput
src/bundle/Resources/public/ts/components/dropdown/dropdown_multi_input.ts TypeScript implementation of multi-value dropdown with checkbox selection

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

const _sourceInputNode = this._sourceNode.querySelector<HTMLSelectElement>('select');

if (!_sourceInputNode) {
throw new Error('DropdownSingleInput: Required elements are missing in the container.');
Copy link

Copilot AI Oct 23, 2025

Choose a reason for hiding this comment

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

Error message incorrectly references 'DropdownSingleInput' but should reference 'DropdownMultiInput' since this is the DropdownMultiInput class.

Suggested change
throw new Error('DropdownSingleInput: Required elements are missing in the container.');
throw new Error('DropdownMultiInput: Required elements are missing in the container.');

Copilot uses AI. Check for mistakes.
this._sourceInputNode.appendChild(option);
});

this.setValue(this._sourceInputNode.value);
Copy link

Copilot AI Oct 23, 2025

Choose a reason for hiding this comment

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

Calling setValue with this._sourceInputNode.value is incorrect for a multi-select. HTMLSelectElement.value returns only the first selected option's value, not all selected values. This line should likely be removed since _value is already initialized from getSelectedValuesFromSource().

Suggested change
this.setValue(this._sourceInputNode.value);
// Removed incorrect setValue call for multi-select.

Copilot uses AI. Check for mistakes.
Base automatically changed from IBX-7918-dropdown to main October 24, 2025 09:10
@GrabowskiM GrabowskiM force-pushed the IBX-7908-multivalue-dropdown branch from 5b6a80f to 918df6d Compare October 24, 2025 13:55
@GrabowskiM GrabowskiM force-pushed the IBX-7908-multivalue-dropdown branch from 918df6d to b875048 Compare October 27, 2025 13:26
@mikadamczyk mikadamczyk requested a review from a team October 30, 2025 13:47
Copy link

@ViniTou ViniTou left a comment

Choose a reason for hiding this comment

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

for php

$items = $this->items;

return array_map(
static function (string $id) use ($items) {
Copy link

Choose a reason for hiding this comment

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

multiple missing callback return typehints here.

);
}

#[ExposeInTemplate('is_empty')]
Copy link

Choose a reason for hiding this comment

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

what this fancy thing do?

Copy link
Contributor

Choose a reason for hiding this comment

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

#[ExposeInTemplate('is_empty')] comes from Symfony UX TwigComponent. It marks the isEmpty() method so that, when the component renders, its return value is automatically available in the Twig template as the variable is_empty. In other words, Twig templates using this component can just write {{ is_empty }} without calling the method explicitly; TwigComponent populates that context entry by running isEmpty().
Without #[ExposeInTemplate('is_empty')], the Twig template would only see the component’s public properties; isEmpty() would remain internal, so you’d have to replicate the empty-check logic in Twig or reference the component object directly (e.g., this.isEmpty()), which is discouraged.


export class DropdownMultiInput extends BaseDropdown {
private _sourceInputNode: HTMLSelectElement;
private _value: string[];
Copy link
Contributor

Choose a reason for hiding this comment

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

I think at the end of our frontend discussion we decided to skip _ for private class properties

@dew326 dew326 merged commit c1f993f into main Oct 31, 2025
5 checks passed
@dew326 dew326 deleted the IBX-7908-multivalue-dropdown branch October 31, 2025 11:30
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.

7 participants