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-967: As an Editor, I want to filter Dropdown items #1891

Merged
merged 5 commits into from
Sep 13, 2021

Conversation

GrabowskiM
Copy link
Contributor

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

Checklist:

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

const visibleItems = this.itemsListContainer.querySelectorAll(`[data-filter-value^="${event.currentTarget.value.toLowerCase()}"]`);

[...allItems].forEach((item) => item.classList.add(CLASS_ITEM_HIDDEN));
[...visibleItems].forEach((item) => item.classList.remove(CLASS_ITEM_HIDDEN));
Copy link
Member

Choose a reason for hiding this comment

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

First it hides all items and in next loop shows matched items.
It may cause some visible artifacts. Can we do it in one loop and check if it should be visible or not inside?

filterItems(event) {
const allItems = this.itemsListContainer.querySelectorAll('[data-filter-value]');

if (event.currentTarget.value.length < 3) {
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be good to assign this condition to some variable to give it a description.

return;
}

const visibleItems = this.itemsListContainer.querySelectorAll(`[data-filter-value^="${event.currentTarget.value.toLowerCase()}"]`);
Copy link
Contributor

Choose a reason for hiding this comment

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

Code may be easier to read after assigning event.currentTarget.value.toLowerCase() to some variable.

@@ -250,6 +260,30 @@
}
}

filterItems(event) {
const allItems = this.itemsListContainer.querySelectorAll('[data-filter-value]');
Copy link
Contributor

Choose a reason for hiding this comment

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

Sometimes (usually?) we add [... ] here, so later it can be skipped.

}

afterClose(event) {
if (event.propertyName === 'transform' && !this.container.classList.contains(CLASS_DROPDOWN_EXPANDED)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I would assign these conditions to variables.

@@ -261,9 +295,12 @@
this.fitItems();

this.container.querySelector(SELECTOR_SELECTION_INFO).onclick = this.onInputClick;
this.itemsContainer
this.itemsContainer.addEventListener('transitionend', this.afterClose, false);
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this work with fast clicking? 😏 😄

}

&--search {
.ibexa-icon {
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, cannot it just be ibexa-icon--small?

{%- endembed -%}
<ul class="ibexa-dropdown__items-list">
{% for choice in preferred_choices %}
{% include '@ezdesign/ui/component/dropdown_item.html.twig' with { choice: choice } %}
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
{% include '@ezdesign/ui/component/dropdown_item.html.twig' with { choice: choice } %}
{% include '@ezdesign/ui/component/dropdown_item.html.twig' with { choice } %}

}

filterItems(event) {
const forceShowItems = event.currentTarget.value.length < 3;
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe we add this to a variable that says what is this value?

@sonarcloud
Copy link

sonarcloud bot commented Sep 13, 2021

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@dew326 dew326 merged commit ba4e158 into master Sep 13, 2021
@dew326 dew326 deleted the IBX-967-filter-dropdown branch September 13, 2021 13:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
5 participants