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

feat(DropdownMenu): added dropdown menu component, created bento menu and update user-profile #299

Merged
merged 1 commit into from
Oct 5, 2021

Conversation

JsGarneau
Copy link
Contributor

@JsGarneau JsGarneau commented Sep 21, 2021

Sorry pour la grosse PR!

Il y a 3 test présentement commenté pour dropdown-menu et dropdown-menu-button. Un concernant le focus et les autre concernant un comportement que je ne suis pas arrivé à reproduire, càd le closeOnClick qui était triggered par onChange, mais étant donner que je travail maintenant avec des children, j'ai pas trop réussi à trouver le pattern pour ca, peut-être que quelqu'un pourrait m'aider.

Il manque aussi la gestion des lozenge, mais ca va venir car le component lozenge n'existe pas encore.

Bug fix checklist

  • Units tests have been adjusted to account for bug.
  • The fix has been tested in multiple Storybook stories.
  • All GitHub checks are successful.

New component checklist

  • The new component and its tests are in the same component folder.
  • The component is unit tested and/or snapshot tested.
  • All of the relevant Storybook stories have been added to the storybook package.
  • There are no linting errors or warnings in the modified/new code.
  • All GitHub checks are successful.

Copy link
Contributor

@alexbrillant alexbrillant left a comment

Choose a reason for hiding this comment

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

partie 1

@alexbrillant
Copy link
Contributor

some pics would be nice

Copy link
Contributor

@maxime-gendron maxime-gendron 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 remarqué que le bouton du Bento n'est pas responsive. Il probablement possible de faire comme j'ai fait dans ma PR #300 et d'utiliser directement le component IconButton.

@Pierregagne Pierregagne self-requested a review September 24, 2021 14:05
@JsGarneau JsGarneau force-pushed the dev/DS-481 branch 2 times, most recently from b3009b3 to 7d6dfec Compare September 27, 2021 14:27
Copy link
Contributor

@Pierregagne Pierregagne 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 fais un check rapide et ça look good :)

@JsGarneau JsGarneau force-pushed the dev/DS-481 branch 5 times, most recently from 7410868 to 7d9b0f4 Compare September 29, 2021 15:57
Copy link
Contributor

@maxime-gendron maxime-gendron left a comment

Choose a reason for hiding this comment

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

Je remarque quelques différences dans les styles des maquettes vs ce qu'on a en ce moment (gauche: maquettes | droite: storybook):

  • Le border radius dans les maquettes est 8px et il est 4pxen ce moment.
  • Les produits sont préfixés Equisoft dans les maquettes.
  • Le logo des produits est plus petit dans les maquettes.
  • Le container du menu est plus large dans les maquettes.
  • L'espacement entre les produits n'est pas le même que dans les maquettes (en ce moment un produit fait 56px de haut, mais dans les maquettes 60px).
  • Les spacings dans la section ressources ne sont pas comme sur les maquettes.

image

@JsGarneau JsGarneau force-pushed the dev/DS-481 branch 2 times, most recently from 37eda71 to f19671a Compare September 30, 2021 20:32
@JsGarneau JsGarneau force-pushed the dev/DS-481 branch 2 times, most recently from 5cb3d89 to c6ac483 Compare October 1, 2021 15:03
Copy link
Contributor

@maxime-gendron maxime-gendron left a comment

Choose a reason for hiding this comment

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

Le container du bento fait 386px au lieu de 384px. Il manque probablement juste box-sizing: border-box sur le container.

Aussi, l'overflow ellipsis ne semble plus fonctionner
image

@JsGarneau JsGarneau force-pushed the dev/DS-481 branch 2 times, most recently from 1cb8db9 to f7d9dae Compare October 5, 2021 13:08
Copy link
Contributor

@maxime-gendron maxime-gendron left a comment

Choose a reason for hiding this comment

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

L'overflow ellipsis ne fonctionne pas en ce moment.

@JsGarneau JsGarneau force-pushed the dev/DS-481 branch 2 times, most recently from 85dc77a to aec3d2c Compare October 5, 2021 18:55
@JsGarneau JsGarneau merged commit 5dfc261 into master Oct 5, 2021
@JsGarneau JsGarneau deleted the dev/DS-481 branch October 5, 2021 19:13
Copy link
Contributor

@LarryMatte LarryMatte left a comment

Choose a reason for hiding this comment

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

External links

  • lorsqu'il y a des ellipsis, le svg est inclus avec le texte et donc il n'est pas affiché. Idéalement il faudrait le voir quand même.
  • Il faudrait aussi centrer l'icône avec le texte.

Screen Shot 2021-10-05 at 3 35 11 PM

Sinon petits commentaires en lien avec les groupes et le css ici et là mais rien de majeur.

Good Job serieux!

@LarryMatte
Copy link
Contributor

LarryMatte commented Oct 5, 2021

User-profile

  • Je ne sais pas pourquoi mais tu as un <label> comme parent de ton user dans le user-profile... À supprimer.
  • le line-height du nom devrait être de 1.4rem. Assures-toi que ça ne vient pas petter ailleurs par contre.
  • Tu peux surement cleaner le code et enlever des span. voir la doc

Screen Shot 2021-10-05 at 4 20 17 PM

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.

5 participants