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: add chat icons #2789

Merged
merged 10 commits into from
Oct 24, 2023
Merged

feat: add chat icons #2789

merged 10 commits into from
Oct 24, 2023

Conversation

gavinbarron
Copy link
Member

Closes #2788

PR Type

  • Bugfix
  • Feature

Description of the changes

adds icons for group chat and meeting chats
fixes overall layout of chat header
refactors ChatHeader in to smaller components
adds a css property for aligning items in the mgt-person fixes padding around chat messages and sendbox

PR checklist

  • Project builds (yarn build) and changes have been tested in at least two supported browsers (Edge + non-Chromium based browser)
  • All public APIs (classes, methods, etc) have been documented following the jsdoc syntax
  • Stories have been added and existing stories have been tested
  • Added appropriate documentation. Docs PR:
  • License header has been added to all new source files (yarn setLicense)
  • Contains NO breaking changes

Other information

adds icons for group chat and meeting chats
fixes overall layout of chat header
refactors ChatHeader in to smaller components
adds a css property for aligning items in the mgt-person
fixes padding around chat messages and sendbox
@microsoft-github-policy-service
Copy link
Contributor

Thank you for creating a Pull Request @gavinbarron.

This is a checklist for the PR reviewer(s) to complete before approving and merging this PR:

  • I have verified a documentation PR has been linked and is approved (or not applicable)
  • I have ran this PR locally and have tested the fix/feature
  • I have verified that stories have been added to storybook (or not applicable)
  • I have tested existing stories in storybook to verify no regression has occured
  • I have tested the solution in at least two browsers (Edge + 1 non-Chromium based browser)

@github-actions
Copy link

🚀 New react-contoso sample application deployed here

@github-actions
Copy link

🚀 New react-contoso sample application deployed here

@github-actions
Copy link

📖 The updated storybook is available here

@github-actions
Copy link

🚀 New react-contoso sample application deployed here

1 similar comment
@github-actions
Copy link

🚀 New react-contoso sample application deployed here

@sebastienlevert
Copy link
Contributor

sebastienlevert commented Oct 20, 2023

  • When using the Edit Group function, some styles have a z-index issue
    image
  • Roster Management lost the pointer on the X. I seems ok on the Add people and Leave buttons.
  • The color behind the "team / group" logo is very "Teams-y". We discussed we would normalize is with web light and dark. Will that be covered by this change?
  • Different left-aligned items have different alignments. @yejuntak can you help here? It seems we need them to be aligned, but right now there are too many padding IMO
    image

@gavinbarron
Copy link
Member Author

gavinbarron commented Oct 20, 2023

Thanks for the review.

  • When using the Edit Group function, some styles have a z-index issue
    image

Oh, that's subtle and I didn't see it! good catch!

  • Roster Management lost the pointer on the X. I seems ok on the Add people and Leave buttons.

Ahh, because they're not buttons anymore, I'll fix this CSS.

  • The color behind the "team / group" logo is very "Teams-y". We discussed we would normalize is with web light and dark. Will that be covered by this change?

It's using the brand colors provided by the fluent provider. At present that's hardcoded to be teamsLightTheme. Let's handle the colors in a separate work item as we don't have a good story for supporting external themes or even toggling between light and dark at present.

  • Different left-aligned items have different alignments. @yejuntak can you help here? It seems we need them to be aligned, but right now there are too many padding IMO
    image

The alignment is wildly inconsistent across the designs, some have the roster management button and the avatar/icon left aligned, some have them with icon for the roster management center aligned with the avatar/icon.
A consistent design would be really useful
There's in the messages shown there is an icon that isn't correctly registered potentially making things worse.

@github-actions
Copy link

📖 The updated storybook is available here

@github-actions
Copy link

🚀 New react-contoso sample application deployed here

@gavinbarron
Copy link
Member Author

@sebastienlevert in the interests of moving this forward I suggest we take theming and padding as separate work items to ensure we don't get stalled on getting this set of improvement merged. This PR is already more complex than I hoped it would be thanks to refactoring a number of components

#2216 already exists for theming and needs definition. Do we need a separate work here to cover dark/light toggling?
#2790 has been created to track the padding issues.

musale
musale previously approved these changes Oct 23, 2023
Copy link
Contributor

@musale musale left a comment

Choose a reason for hiding this comment

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

Good to go.

@github-actions
Copy link

📖 The updated storybook is available here

@github-actions
Copy link

🚀 New react-contoso sample application deployed here

@gavinbarron gavinbarron dismissed stale reviews from sebastienlevert and musale via 6de8d58 October 23, 2023 14:57
@github-actions
Copy link

📖 The updated storybook is available here

@github-actions
Copy link

🚀 New react-contoso sample application deployed here

@github-actions
Copy link

📖 The updated storybook is available here

@gavinbarron gavinbarron enabled auto-merge (squash) October 23, 2023 18:15
@gavinbarron gavinbarron merged commit 112aaba into next/mgt-chat Oct 24, 2023
8 checks passed
@gavinbarron gavinbarron deleted the fix/chat-header branch October 24, 2023 11:25
@yejuntak
Copy link
Contributor

yejuntak commented Oct 24, 2023

  • Different left-aligned items have different alignments. @yejuntak can you help here? It seems we need them to be aligned, but right now there are too many padding IMO
    image

The alignment is wildly inconsistent across the designs, some have the roster management button and the avatar/icon left aligned, some have them with icon for the roster management center aligned with the avatar/icon. A consistent design would be really useful There's in the messages shown there is an icon that isn't correctly registered potentially making things worse.

I have made the all the alignment to left and made padding to 20px on both short and wide form in the Figma file. Please let me know this helped.

@gavinbarron gavinbarron linked an issue Oct 25, 2023 that may be closed by this pull request
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

[mgt-chat] add icons for group and meeting chats
4 participants