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

[EASI-4549] System workspace "Team" card #2778

Merged
merged 12 commits into from
Aug 28, 2024
Merged

[EASI-4549] System workspace "Team" card #2778

merged 12 commits into from
Aug 28, 2024

Conversation

adamodd
Copy link
Contributor

@adamodd adamodd commented Aug 24, 2024

EASI-4549

https://www.figma.com/design/z8GeZPDfIh9oKfHK1cX7Fo/System-centric-EASi?node-id=4239-8187&t=b9iFjrGAECM00YdC-0

Description

  • New SystemWorkspace/TeamCard
  • Sort the team member roles by teamRolesIndex
  • Lifted Avatar from Mint
  • Button will link up in later tickets

How to test this change

  • Unit test snapshot with the rendered team list
  • Seed and launch local dev
    • View a system in My Systems
      • Select a system to see the Workspace page with the new TeamCard

PR Author Checklist

  • I have provided a detailed description of the changes in this PR.
  • I have provided clear instructions on how to test the changes in this PR.
  • I have updated tests or written new tests as appropriate in this PR.

PR Reviewer Guidelines

  • It's best to pull the branch locally and test it, rather than just looking at the code online!
  • When approving a PR, provide a reason why you're approving it
    • e.g. "Approving because I tested it locally and all functionality works as expected"
    • e.g. "Approving because the change is simple and matches the Figma design"
  • Don't be afraid to leave comments or ask questions, especially if you don't understand why something was done! (This is often a great time to suggest code comments or documentation updates)
  • Check that all code is adequately covered by tests - if it isn't feel free to suggest the addition of tests.

@adamodd adamodd marked this pull request as ready for review August 27, 2024 02:21
@adamodd adamodd requested a review from a team as a code owner August 27, 2024 02:21
@adamodd adamodd requested review from aterstriep and removed request for a team August 27, 2024 02:21
Copy link
Contributor

Choose a reason for hiding this comment

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

We have an existing InitialsIcon component that we use in a couple of other places. Can you update those instances to use Avatar instead since the ticket says users should have the same color across the app? That way we can go ahead and remove InitialsIcon so we don't have two mostly duplicate components.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay didn't notice that. Avatar has the new colors we want? And swapping out InitialsIcon is probably good to go?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just went for it. The only thing I caught was this font-body-2xs difference

`easi-initials-icon display-flex flex-align-center flex-justify-center font-body-2xs circle-4 ${colorClass}`,

Not sure if it should've stayed. If so looks ready to pass back in.

src/views/SystemWorkspace/components/TeamCard/index.tsx Outdated Show resolved Hide resolved
Copy link
Contributor

Choose a reason for hiding this comment

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

Good idea to bring this up into a util function 👍🏻

Copy link
Contributor

@aterstriep aterstriep left a comment

Choose a reason for hiding this comment

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

Changes look good 👍🏻

@adamodd adamodd merged commit 31d2ddd into main Aug 28, 2024
12 checks passed
@adamodd adamodd deleted the EASI-4549/ws_team_card branch August 28, 2024 15:05
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.

2 participants