Skip to content

Improve facet limitation#41

Open
ThibaudGLT wants to merge 1 commit intomasterfrom
improve-facet-limitation
Open

Improve facet limitation#41
ThibaudGLT wants to merge 1 commit intomasterfrom
improve-facet-limitation

Conversation

@ThibaudGLT
Copy link
Copy Markdown
Contributor

  • Add a collapse button with specific label setting
  • Fix behavior when search is updated with new selected facet
  • Add reset button
  • Change filter button icon

- Add a collapse button with specific label setting
- Fix behavior when search is updated with new selected facet
- Add reset button
- Change filter button icon
@ThibaudGLT ThibaudGLT requested a review from jajm March 18, 2026 15:27
Comment on lines +24 to +38
function toggleHiddenFacets(name, button) {
var hiddenFacets = document.querySelector('.search-facet[data-facet-name="' + name + '"] .hidden-facets');

if (hiddenFacets.style.display === 'none' || hiddenFacets.style.display === '') {
hiddenFacets.style.display = 'block';
button.classList.remove('o-icon-down');
button.classList.add('o-icon-up');
button.innerHTML = ' ' + button.getAttribute('data-collapse-label');
} else {
hiddenFacets.style.display = 'none';
button.classList.remove('o-icon-up');
button.classList.add('o-icon-down');
button.innerHTML = ' ' + button.getAttribute('data-expand-label');
}
}
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

C'est pas vraiment "theme-friendly": modif de l'icône et de la propriété display + le fait de se baser sur la propriété display pour savoir s'il faut cacher ou montrer le bloc.

Ça annule ce que j'avais fait dans 9c31e68 (où le JS se contente d'ajouter/retirer une classe, tout le reste est fait en CSS)

Pour le bouton qui change d'icône et de libellé, c'est à mon avis plus simple (et plus theme-able) d'avoir 2 boutons, et de cacher l'un ou l'autre selon le besoin.
Ça évite aussi une faille XSS (Si data-*-label contient une balise <script> par ex, c'est exécuté quand on clique sur le bouton)

}
for (const [key, value] of Array.from(params.entries())) {
if (key.startsWith('limit[')) {
params.delete(key);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

En théorie on peut utiliser les paramètres limit pour autre chose que les facettes visibles.
Je pense que c'était mieux avant puisque ça ne supprimait que les paramètres des facettes visibles.
Pourquoi avoir changé d'ailleurs ? Ça ne marchait pas ?

Comment on lines +61 to +63
allInputs.forEach(input => {
input.checked = false;
});
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

À quoi ça sert de décocher puisqu'on redirige juste après (location.search = ...) ?

Comment on lines +68 to +72
for (let [key, value] of params.entries()) {
if (!key.startsWith('limit[')) {
newParams.append(key, value);
}
}
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Même remarque que pour submitFacets, on devrait conserver les paramètres limit qui ne correspondent pas à une facette visible

Comment on lines +43 to +44
<button class="fas fa-filter" id="submit-facets" title="<?php echo $this->translate('Filter with selected facets');?>"></button>
<button class="fas fa-times" id="reset-facets" title="<?php echo $this->translate('Reset all filters');?>"></button>
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Il manque escapeHtml

Comment on lines +75 to 78
data-expand-label="<?php echo $this->escapeHtml($this->searchCurrentPage()->settings()['facet_limit_expand_label']); ?>"
data-collapse-label="<?php echo $this->escapeHtml($this->searchCurrentPage()->settings()['facet_limit_collapse_label']); ?>">
&nbsp;<?php echo $this->escapeHtml($this->searchCurrentPage()->settings()['facet_limit_expand_label']); ?>
</button>
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Tu peux utiliser $settings (déclaré plus haut) à la place de $this->searchCurrentPage()->settings()

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.

2 participants