Skip to content
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

IBX-2622: As a developer I want to have dynamic Dropdown #381

Merged
merged 4 commits into from
Apr 5, 2022

Conversation

GrabowskiM
Copy link
Contributor

Question Answer
Tickets https://issues.ibexa.co/browse/IBX-2622
Bug fix? no
New feature? yes
BC breaks? no
Tests pass? yes
Doc needed? no, I think
License GPL-2.0

Checklist:

  • Coding standards ($ composer fix-cs)
  • Ready for Code Review

@@ -283,6 +289,65 @@
return this.itemsContainer;
}

hasChangedOptions(mutationList) {
return mutationList.reduce(
Copy link
Contributor

Choose a reason for hiding this comment

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

some()?

Comment on lines 300 to 306
if (this.canSelectOnlyOne) {
const selectedItem = this.sourceInput.querySelector(`[value="${this.sourceInput.value}"]`);

return selectedItem ? [selectedItem] : [];
}

return [...this.sourceInput.querySelectorAll('.ibexa-input:checked')];
Copy link
Contributor

Choose a reason for hiding this comment

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

<option>s are not ibexa-input and :checked should also work for single select

Suggested change
if (this.canSelectOnlyOne) {
const selectedItem = this.sourceInput.querySelector(`[value="${this.sourceInput.value}"]`);
return selectedItem ? [selectedItem] : [];
}
return [...this.sourceInput.querySelectorAll('.ibexa-input:checked')];
return [...this.sourceInput.querySelectorAll(':checked')];

@@ -283,6 +289,65 @@
return this.itemsContainer;
}

hasChangedOptions(mutationList) {
Copy link
Contributor

Choose a reason for hiding this comment

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

wdyt?

Suggested change
hasChangedOptions(mutationList) {
haveOptionsChanged(mutationList) {

Comment on lines 327 to 328
if (!newOptions.length) {
this.container.classList.add('ibexa-dropdown--disabled');
Copy link
Contributor

Choose a reason for hiding this comment

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

If you remove all options (dropdown becomes disabled) and then add a new one it won't enable itself again.

Suggested change
if (!newOptions.length) {
this.container.classList.add('ibexa-dropdown--disabled');
this.container.classList.toggle('ibexa-dropdown--disabled', !newOptions.length);


createOption(value, label) {
const container = doc.createElement('div');
const renderedTemplate = this.itemTemplate.replaceAll('{{ value }}', value).replaceAll('{{ label }}', label);
Copy link
Contributor

Choose a reason for hiding this comment

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

nitpick

Suggested change
const renderedTemplate = this.itemTemplate.replaceAll('{{ value }}', value).replaceAll('{{ label }}', label);
const itemRendered = this.itemTemplate.replaceAll('{{ value }}', value).replaceAll('{{ label }}', label);

}

recreateOptions() {
const newOptions = this.sourceInput.querySelectorAll('option');
Copy link
Contributor

Choose a reason for hiding this comment

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

For me new is misleading, because, not all options are actually new.

@GrabowskiM GrabowskiM requested review from tischsoic and dew326 April 4, 2022 10:52
@GrabowskiM GrabowskiM force-pushed the IBX-2622-dynamic-dropdown branch from ac8eab1 to f872e6f Compare April 4, 2022 11:12
@dew326 dew326 merged commit 8be701c into main Apr 5, 2022
@dew326 dew326 deleted the IBX-2622-dynamic-dropdown branch April 5, 2022 07:33
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.

4 participants