Skip to content

The user can freely choose statuses an item heal by using the multiselect component #484

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

Merged
merged 11 commits into from
Jun 9, 2025

Conversation

AntoinePoree
Copy link
Collaborator

@AntoinePoree AntoinePoree commented Apr 9, 2025

Thank you for your contribution to the Pokémon Studio repo.

Before submitting this PR into the develop branch, please make sure:

  • Your code builds clean without any errors or warnings
  • You are following the Code guidelines
  • You tested your code to make sure it does what it is supposed to do

Description

  • Add a new select, the Multi select.
  • The users would like to freely choose the statuses that an item heals

Note before testing

Multiselect is a derived of Select, some functionnality of Select has be taken. Bug too certainly

Tests to perform

  • Test the multiselect to see if he respond correctly

#400

@AntoinePoree AntoinePoree self-assigned this Apr 9, 2025
@AntoinePoree AntoinePoree marked this pull request as draft April 9, 2025 18:40
Improve checkbox to get an indeterminate status
Remove None and ALL front status Heal
@AntoinePoree AntoinePoree marked this pull request as ready for review April 11, 2025 21:22
Copy link
Collaborator

@AerunDev AerunDev left a comment

Choose a reason for hiding this comment

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

Hello !
J'ai pu tester cette branche avec le MultiSelect et ça fonctionne bien, bien joué ! 😄
L'idée d'adapter le bloc de données sur la vue lecture avec les éléments sélectionnés est une bonne idée. Il faudra qu'on soit vigilant à ne pas le faire partout car si la liste comprend 20 éléments ça peut vite être problématique.

Je me permets quelques remarques pour améliorer le rendu et le fonctionnel :

  • Sur le Select de base, on peut utiliser les flèches HAUT / BAS pour parcourir la liste, et ENTRÉE pour valider. Dans le cas du MultiSelect, on s'attendrait à pouvoir faire de même, avec ENTRÉE qui coche la Checkbox sans refermer l'input. On peut choisir d'ignorer cette partie mais je pense que pour des raisons d'accessibilité c'est important.
  • Quand la liste déroulante est affichée sous l'input, on s'attend à ce qu'en cliquant sur l'input elle soit fermée à nouveau. Est-ce qu'on ajoute ce comportement ?
  • Si on se fie au Figma, l'état d'un élément de la liste dont la checkbox est cochée affiche bien un fond plus clair, mais seulement si on est en hover avec la souris. Dans le cas où la souris n'est pas dessus, on ne devrait pas avoir le fond plus clair. Je mets une capture pour plus de clarté.
    image
  • D'après les discussions qu'on avait eu avec Walven, et les éléments retenus sur Discord ensemble, le comportement pour l'input devrait être le suivant :
    • L'input s'adapte à la sélection en concaténant les valeurs de la façon suivante : Valeur 1, Valeur 2, Valeur 3
    • L'input affiche 3 lignes max. Donc si le texte concaténé dans l'input dépasse 3 lignes, on affiche un text-overflow: ellipsis "...".
    • Ce qui permet au final d'afficher quelque chose de plus explicite que "8 sélectionnés".
  • L'icône de la Checkbox "-" n'est pas centrée, dans le cas où une partie des éléments de la liste est sélectionnée.

Je vous laisse réagir à ce retour, notamment @Walven, comme ça on évite les allers retours trop conséquents et on se met d'accord avant corrections / ajouts.
Bien joué encore ! 🎉

@AerunDev AerunDev linked an issue Apr 14, 2025 that may be closed by this pull request
2 tasks
@AerunDev AerunDev changed the title feat(multiselect): create the component The user can freely choose statuses an item heal by using the multiselect component Apr 14, 2025
@invatorzen
Copy link

Just wanted to say I tested this and it works at properly assigning multiple statuses to the already existing array "statusList"!

…d styling

- Updated button click handlers to prevent event propagation.
- Replaced MultiSelectInput with MultiSelectTextArea for improved user experience.
- Adjusted MultiSelectContainer styles for better layout and responsiveness.
- Implemented keyboard navigation and selection highlighting in RenderOptions.
- Enhanced input resizing logic for better display of selected values.
- Cleaned up unused code and improved overall structure for maintainability.
@AntoinePoree
Copy link
Collaborator Author

J'ai énormément retravaillé le composant, il est dans l'état, très différent de sa première version.

Sinon, j'ai pris en compte tes nombreux retours Aerun, normalement, tu devrais être plus content de cette version.

Thanks for testing previously, but we need to test it again after my last review 😇

Copy link
Collaborator

@AerunDev AerunDev left a comment

Choose a reason for hiding this comment

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

Hello ! 😄
J'ai pris le temps de tester ta dernière proposition, et c'est vraiment chouette, bien joué !

J'ai quelques petits détails à soulever tout de même pour qu'on soit carrés.

  1. Quand on sélectionne suffisamment d'éléments pour que l'input affiche 3 lignes, l'espace entre le bas de l'input et le texte semble trop petit vis à vis de l'espace du dessus.
    image

  2. Si je vais sur l'objet Antidote et que je sélectionne tous les statuts de la liste, tout est bon au niveau des checkboxes. Idem si je choisis "Tout sélectionner" à la souris. Par contre, si j'utilise le clavier (HAUT / BAS puis ENTRÉE) sur "Tout sélectionner", l'icône de Checkbox est sélectionnée mais l'icône de check n'est pas visible (cf. la capture d'écran).
    image

  3. Quand je sélectionne certains statuts, que je sors de la modale d'édition, et que je reclique sur le bloc "Soin" pour modifier la liste à nouveau, l'input semble afficher une troisième ligne vide. Je ne sais pas si c'est le même symptôme qu'on avait à un moment, mais il manque peut-être un refresh à un moment. Peut-être attendre une review de code avant d'attaquer ce problème ?
    image

…tionality

- Increased max-height of MultiSelectTextArea for better usability.
- Introduced determineCheckboxState function for clearer checkbox state management.
- Enhanced input resizing logic to accommodate new height adjustments.
- Added label truncation for long selections in getSelectDefaultLabel function.
- Updated Checkbox styles to handle indeterminate state visibility.
Copy link
Collaborator

@AerunDev AerunDev left a comment

Choose a reason for hiding this comment

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

J'ai pu rester, et on est tout bon pour moi, let's goooo ! 🥳

Copy link
Collaborator

@Palbolsky Palbolsky left a comment

Choose a reason for hiding this comment

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

Quand on clique sur la scrollbar, le select se ferme alors que ça ne devrait pas être le cas.
Donc on ne peut pas utiliser la scrollbar.

…mprove MultiSelectTextArea props

- Added mouse event handling to prevent default behavior in RenderOptions for better interaction.
- Removed unnecessary `tabIndex` from MultiSelectTextArea to streamline props usage.
Copy link
Collaborator

@Palbolsky Palbolsky left a comment

Choose a reason for hiding this comment

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

C'est bon pour moi GG :party

Copy link
Collaborator

@Aelysya Aelysya left a comment

Choose a reason for hiding this comment

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

Ca marche nickel 👌
Juste un petit détail, est-ce qu'on mettrai pas 'tous les status' quand ils sont tous selectionnés au lieu de faire une liste ?

@AerunDev
Copy link
Collaborator

AerunDev commented Jun 5, 2025

Ca marche nickel 👌 Juste un petit détail, est-ce qu'on mettrai pas 'tous les status' quand ils sont tous selectionnés au lieu de faire une liste ?

Si effectivement on peut faire ça. Ça éviterait de devoir ouvrir le Select pour vérifier qu'on a tous les statuts de sélectionnés.

… and tooltip support

- Added 'All statuses' translation to English and French language files.
- Updated MultiSelect component to accept a tooltip for the 'select all' option.
- Enhanced getSelectDefaultLabel function to return a custom label when all options are selected.
@AerunDev AerunDev merged commit ca35101 into develop Jun 9, 2025
5 checks passed
@AerunDev AerunDev deleted the feat-400-multiselect-heal-status branch June 9, 2025 15: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.

The user can freely choose the statuses an item heals
5 participants