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

upcoming: [M3-7611] - Placement Groups Detail Summary #10164

Merged
merged 11 commits into from
Feb 12, 2024

Conversation

carrillo-erik
Copy link
Contributor

@carrillo-erik carrillo-erik commented Feb 8, 2024

Description 📝

This PR introduces the PlacementGroupsSummary component.

Changes 🔄

  • Add Summary panel which displays the Placement Group's configuration details.
  • Add logic to display a warning notice in the even the Placement Group is not compliant.
  • Add unit testing for component.

Preview 📷

Placement Group Summary View

pg-summary

Tooltip

pg-summary-tooltip

Non-compliant Placement Group Notice

Screenshot 2024-02-09 at 6 09 41 AM

How to test 🧪

Prerequisites

(How to setup test environment)

  • Enable the following using the Cloud Manager Dev Tools:
    • Placement Groups feature flag
    • MSW

Verification steps

(How to verify changes)

  • Navigate to http://localhost:3000/placement-groups
  • Click on a Placement Group from the table.
  • Verify the configuration info to match the info from the Placement Group you clicked.
  • Verify that the tooltip is rendered and behaves as expected.
  • Run the following unit test: yarn test PlacementGroupsSummary

As an Author I have considered 🤔

Check all that apply

  • 👀 Doing a self review
  • ❔ Our contribution guidelines
  • 🤏 Splitting feature into small PRs
  • ➕ Adding a changeset
  • 🧪 Providing/Improving test coverage
  • 🔐 Removing all sensitive information from the code and PR description
  • 🚩 Using a feature flag to protect the release
  • 👣 Providing comprehensive reproduction steps
  • 📑 Providing or updating our documentation
  • 🕛 Scheduling a pair reviewing session
  • 📱 Providing mobile support
  • ♿ Providing accessibility support

@carrillo-erik carrillo-erik self-assigned this Feb 8, 2024
@carrillo-erik carrillo-erik requested a review from a team as a code owner February 8, 2024 16:37
@carrillo-erik carrillo-erik requested review from mjac0bs and cpathipa and removed request for a team February 8, 2024 16:37
Copy link

github-actions bot commented Feb 8, 2024

Coverage Report:
Base Coverage: 81.13%
Current Coverage: 81.13%

Copy link
Contributor

@abailly-akamai abailly-akamai left a comment

Choose a reason for hiding this comment

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

@carrillo-erik looks good - you need to:

  • add a changeset
  • fix the unit failing
  • add a test for the PlacementGroupsSummary.tsx

also:
Screenshot 2024-02-08 at 15 28 29

  • The spacing is off for the non-compliance banner.
  • The learn more link in the banner is probably not accessible

@carrillo-erik
Copy link
Contributor Author

@carrillo-erik looks good - you need to:

  • fix the unit failing
  • add a test for the PlacementGroupsSummary.tsx
  • The spacing is off for the non-compliance banner.
  • Fixed the unit test that was failing and added unit test for PlacementGroupsSummary.tsx
  • I refactored the Notice and the spacing was addressed to reflect the mock up in better detail.
  • The learn more link in the banner is probably not accessible
  • Do you mean as in the link/location is not ready yet?

@abailly-akamai
Copy link
Contributor

@carrillo-erik no, I wonder what we do for those dark blue links in dark mode (the "Learn more" in this case). Cause there is not enough contrast for it to be accessible and legible

@carrillo-erik
Copy link
Contributor Author

@carrillo-erik no, I wonder what we do for those dark blue links in dark mode (the "Learn more" in this case). Cause there is not enough contrast for it to be accessible and legible

Oh sorry, I misunderstood your comment. Indeed, our a11y does suffer quite a bit in dark mode. One suggestion, for the links in particular, could be that we adopt the same guidelines as GitHub. Recently they made the change to show the underline for links by default. The contrast ratio would still require some work, however, I believe they would be more legible.

@abailly-akamai
Copy link
Contributor

@carrillo-erik can you check with UX and/or what we do in those situations through the rest of the UI?

Copy link
Contributor

@abailly-akamai abailly-akamai left a comment

Choose a reason for hiding this comment

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

Almost there. just a few fixes for cleanup and consistency

Learn more.
</Link>
</Typography>
</Notice>
Copy link
Contributor

Choose a reason for hiding this comment

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

The Notice should live inside the PlacementGroupSummary component

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@abailly-akamai I had originally done this, however, having the <Notice /> inside the <PlacementGroupSummary /> was the reason why the unit test was failing. This led me to rethink about the component composition and I feel the <Notice /> should live outside of the <PlacementGroupSummary /> as this component is only meant to display the config details. It's a personal preference but if you believe otherwise I can make the change.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think that test failure should warrant this change. The notice only pertains to the summary so it should be there. You put it in the SafeTabPanel which isn't right. Let's put it back inside the summary and fix the test. I can help if you can't find the source of the failure

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Made the changes.

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 didn't use a constant and instead incorporated the string right in the component, this seems to have solved the issue I was encountering with the unit test.

packages/manager/src/features/PlacementGroups/utils.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@mjac0bs mjac0bs left a comment

Choose a reason for hiding this comment

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

Looks good - summary page displays the expected data based on the mocked API response. Tests are passing. Left a couple very minor suggestions.

Unrelated to this PR, I also noted that the text tooltip in dark mode appears to be the wrong link color. Might be worth its own follow up ticket.

Screenshot 2024-02-09 at 2 05 29 PM

packages/manager/src/features/PlacementGroups/constants.ts Outdated Show resolved Hide resolved
import { PlacementGroupsSummary } from './PlacementGroupsSummary';

describe('PlacementGroups Summary', () => {
it('renders the placement group detail summary panel', () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Optional: we could also check if the tooltip exists.

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 included this check in the unit test.

@mjac0bs mjac0bs added the Approved Multiple approvals and ready to merge! label Feb 9, 2024
@carrillo-erik carrillo-erik merged commit b025e40 into linode:develop Feb 12, 2024
18 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Approved Multiple approvals and ready to merge! Placement Groups
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants