search

microsoftgraph / microsoft-graph-toolkit

Authentication Providers and UI components for Microsoft Graph 🦒
https://docs.microsoft.com/graph/toolkit/overview
Other
964 stars 309 forks source link

[BUG] - Alignment of details lines in Person component is no longer centered when using a large profile photo #2976

Closed JHolah closed 10 months ago

JHolah commented 10 months ago

Describe the bug The new version no longer applies center-aligned content when the profile image is larger than the vertical space occupied by the details text.

To Reproduce Steps to reproduce the behavior:

  1. Go to MGT playground Person component: https://mgt.dev/?path=/story/components-mgt-person--person
  2. Insert the following code into the HTML section:

<mgt-person person-query="me" view="threeLines" class="example" person-card="hover"></mgt-person>

  1. Insert the following code into the CSS section:
.example {
      --person-avatar-size: 120px
}
  1. Note that the text is not vertically-aligned:

v3align

Expected behavior The image and text should be center aligned:

v2align

CSS which handled the alignment no longer exists in the new version:

new:

:host .flyout [slot="anchor"] {
    display: flex;
    cursor: pointer;
}

old:

:host .person-root, mgt-person .person-root {
    position: relative;
    display: flex;
    align-items: center;
    color: var(--color,var(--color-sub1,#323130));
}

Environment (please complete the following information):

Additional context Add any other context about the problem here.

Rafaelki commented 10 months ago

Note that it works fine (vertically centered) if the Person component doesn't have the person-card property, but as the Person card adds a wrapping div, the alignment stops working.

sebastienlevert commented 10 months ago

Oh. This is a good catch! Thanks for reporting it! And thanks @Rafaelki for the additional insights! This is a bug and should be fixed. Thanks!

If you ever feel like it @JHolah, we are happy to accept PR with the fixes! If not, we'll queue it for a fix after our v4 release that should shortly happen. Thanks!

gavinbarron commented 10 months ago

Thanks @JHolah and @Rafaelki!

I really appreciate the quality of issues from you both, it makes like as a maintainer much easier when we get super clear repos and descriptions of the problems you all are facing.

gavinbarron commented 10 months ago

I just tested this with the next version the playground https://mgt.dev/next/?path=/story/components-mgt-person-html--person and it looks like this was resolved in #2900