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

Button default options #1246

Open
wants to merge 44 commits into
base: next
Choose a base branch
from
Open

Button default options #1246

wants to merge 44 commits into from

Conversation

ThierryNormand
Copy link
Contributor

New button used to select the default options for the search results.

Can revert all options back to default or only revert specified group.

Button next to Select All button.

@pelord pelord requested a review from alecarn January 31, 2024 15:30
Copy link
Collaborator

@alecarn alecarn left a comment

Choose a reason for hiding this comment

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

  1. Pour le UI, je trouve que le bouton n'a pas sa place dans un menu ou il n'y a aucune sélection de disponible ou de visible.
    image
  2. Est-ce que ce serait intéressant d'utiliser un master checkbox qui permet de tout cocher/décocher et indique un état intermédiaire lorsque ce n'est pas le cas?
    Par exemple:
    image
  3. On pourrait p-t améliorer l'intégration du bouton "Sélection par défaut" en utilisant un bouton icone?
    image

Sinon une non-conformité, le menu sort de l'écran
image

// This component is projected, the style need to be apply outside is scope
.checkDefaultButton {
text-align: center;
padding: 0 5px;
Copy link
Collaborator

Choose a reason for hiding this comment

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

En général avec Material design, on essaie de respecter les multiples de 4 ou au moins en nombre paires pour nos espacements (padding/margin).
Voir le Material measurements

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Je me base sur des éléments déjà existants comme le bouton de tout sélectionner qui utilise ce multiple

Copy link
Collaborator

Choose a reason for hiding this comment

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

Je crois qu'il faut se conformer ici

setting: SearchSourceSettings
) {
event.stopPropagation();
setting.allEnabled = true;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Pourquoi mettre le allEnabled à vrai?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sinon, ça crée un état où le bouton de sélection par défaut sélectionnait tout lorsque tout était désélectionné.

const limit =
this.options.params && this.options.params.limit
? Number(this.options.params.limit)
!forceReset && this.options.params && this.options.params.limit
Copy link
Collaborator

Choose a reason for hiding this comment

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

Pourquoi le forceReset, dans quel scénario on pourrait avoir des résultats par défaut qui sont différent?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

C'est pour le bouton qui remet tous les dépendances par défaut. Ce bouton n'a pas les settings dans son container. Sans le forceReset, les valeurs par défaut des radio button changeaient selon l'option sélectionnée.

) {
event.stopPropagation();
setting.allEnabled = true;
this.checkUncheckAll(event, source, setting);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Pourquoi faire un uncheck si on repasse tous les settings ci-dessous. Surtout qu'on va générer deux searchSourceChange.emit()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Dépendamment de la valeur du bouton de tout sélectionner, un état faisait que le bouton de sélection par défaut ne fonctionnait pas. Ça "reset" en quelque sorte.

Comment on lines +220 to +226
source.getDefaultOptions().settings.map((defaultSetting) => {
if (defaultSetting.title === setting.title) {
setting.values.map((value, index) => {
value.enabled = defaultSetting.values[index].enabled;
});
}
});
Copy link
Collaborator

Choose a reason for hiding this comment

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

J'ai l'impression qu'on pourrait optimiser et améliorer la lisibilité? Je n'ai pas testé!

Suggested change
source.getDefaultOptions().settings.map((defaultSetting) => {
if (defaultSetting.title === setting.title) {
setting.values.map((value, index) => {
value.enabled = defaultSetting.values[index].enabled;
});
}
});
const defaultSetting = source.getDefaultOptions().settings.find(category => category.title === setting.title);
setting.values = defaultSetting.values;

@pelord
Copy link
Member

pelord commented Feb 2, 2024

  1. Pour le UI, je trouve que le bouton n'a pas sa place dans un menu ou il n'y a aucune sélection de disponible ou de visible.
    image
  2. Est-ce que ce serait intéressant d'utiliser un master checkbox qui permet de tout cocher/décocher et indique un état intermédiaire lorsque ce n'est pas le cas?
    Par exemple:
    image
  3. On pourrait p-t améliorer l'intégration du bouton "Sélection par défaut" en utilisant un bouton icone?
    image

Sinon une non-conformité, le menu sort de l'écran image

Pour le 1, ca reste intéressant de tout resetter sans avoir a faire le tour de chaque search source.
Pour les 2,3,4 et.. ca doit etre considéré je crois!

@pelord pelord changed the base branch from next to release/16.x March 13, 2024 14:05
@pelord pelord changed the base branch from release/16.x to next April 12, 2024 15:42
@@ -50,7 +50,7 @@ export class CadastreSearchSource extends SearchSource implements TextSearch {
/*
* Source : https://wiki.openstreetmap.org/wiki/Key:amenity
*/
protected getDefaultOptions(): SearchSourceOptions {
getDefaultOptions(): SearchSourceOptions {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Pourquoi enlever le protected?

// This component is projected, the style need to be apply outside is scope
.checkDefaultButton {
text-align: center;
padding: 0 5px;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Je crois qu'il faut se conformer ici

Copy link
Collaborator

@alecarn alecarn left a comment

Choose a reason for hiding this comment

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

Faudrait faire le tour des commentaire et on a des erreurs de typage

@@ -118,16 +118,15 @@ export class IChercheSearchSource extends SearchSource implements TextSearch {
return IChercheSearchSource.type;
}

protected getDefaultOptions(): SearchSourceOptions {
getDefaultOptions(forceReset?: Boolean): SearchSourceOptions {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Erreur de typage

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.

3 participants