microsoftgraph / microsoft-graph-toolkit

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

[`mgt-chat`] new chat design should be consistent #2782

Closed musale closed 11 months ago

musale commented 1 year ago

Describe the bug Current image

Expected

image

gavinbarron commented 1 year ago

@musale we should take care to test the new Chat component in the react-chat test application and work out how we should best provide a good story for embedding it in dialogs like this as an extension.

Perhaps we add a prop, that when set causes the container element to render without the current padding, background-color, and box-shadow CSS rules to make it embed in react-contoso better?

@sebastienlevert any thoughts on this?

sebastienlevert commented 1 year ago

In this case, I don't think the elevation is required here. I feel this is up to the design system of the developer to decide on how this gets elevated in their scenarios.

gavinbarron commented 1 year ago

So, no elevation at all? or just in the case where a developer wants to embed it?

I think that having elevation makes sense when the component is used with the title showing given the current design

gavinbarron commented 12 months ago

ping @sebastienlevert

sebastienlevert commented 12 months ago

Adding @yejuntak. I feel elevation is tricky around a component. I still think this is a developers job to make it fit with their design system... Peter, can you comment here on what should be the right behavior?

yejuntak commented 12 months ago

This seems to be very tricky because having shadows over the title and having no shadows are wrong behaviors. For alternative, I agree with Seb on removing the elevation for now.

gavinbarron commented 12 months ago

Ok I can make that fly.

musale commented 12 months ago

Also, I should've added that the title text left padding to be aligned with the elements below it and have a divider running between it and the "To" text in the expected image.

gavinbarron commented 11 months ago

We are now removing the header text, divider, padding, and elevation from the component