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-7608]: Placement Groups Landing page empty state #10075

Merged
merged 4 commits into from
Jan 22, 2024

Conversation

carrillo-erik
Copy link
Contributor

@carrillo-erik carrillo-erik commented Jan 18, 2024

Description πŸ“

Empty state UI on the landing page for the upcoming Placement Groups feature.

Changes πŸ”„

  • Introduces a unit test for the landing page empty state.

Preview πŸ“·

Light Mode Dark Mode
pg-light pg-dark
Unit Test
pg-test

How to test πŸ§ͺ

Prerequisites

  • Follow these steps to test the changes in this PR before Alban's PG Landing Page PR is merged.
  1. Checkout the Alban's draft PR
  2. Pull the changes from this PR down.
  3. Open the serverHandlers.ts file and change line no. 1875 as follows: return res(ctx.json(makeResourcePage(placementGroupFactory.buildList(0))));
  4. Run the project and turn on the MSW.
  • If testing after Alban's PG Landing Page PR is merged, only follow steps 3 and 4, listed above.

Verification steps

  • Run the unit test yarn test path/to/cloud-manager/manager/packages/manager/src/features/PlacementGroups/PlacementGroupsLanding/PlacementGroupsLanding.test.tsx
  • Visually inspect the UI for any defects.

As an Author I have considered πŸ€”

  • πŸ‘€ 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 Jan 18, 2024
@carrillo-erik carrillo-erik requested a review from a team as a code owner January 18, 2024 15:16
@carrillo-erik carrillo-erik requested review from dwiley-akamai and abailly-akamai and removed request for a team January 18, 2024 15:16
};

export const gettingStartedGuides: ResourcesLinkSection = {
links: [
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The mock ups do not show the typical resource section for empty state. I'm going to post on the project slack channel for insight on future plans. For now, the fields were set to empty strings.

@carrillo-erik
Copy link
Contributor Author

This PR is dependent on this PR. In order to commit my changes in the current state, I had to bypass the husky pre-commit hook. Changes will need to be verified before merging this PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

You can just delete that - we can test your component as a stand-alone manually or wait for dependent PR to be merged

Copy link
Contributor

Choose a reason for hiding this comment

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

You don't really need a .tsx extension if you don't have and JSX in there - can just use .ts

Copy link
Contributor

@dwiley-akamai dwiley-akamai left a comment

Choose a reason for hiding this comment

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

Unit test passes βœ…

Confirmed empty state renders βœ…

Screenshot 2024-01-18 at 1 09 03 PM


export const headers: ResourcesHeaders = {
description:
'Control the physical placement or distribution of virtual machines (VMs) instances within a data center or availability zone.',
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this final copy? "[...] virtual machines (VMs) instances" sounds a bit odd to me. I think it'd sound better either removing "instances" or changing "machines" --> "machine" (& "VMs" --> "VM")

Copy link
Contributor

Choose a reason for hiding this comment

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

We're onboarding a tech writer this week and we'll a lot of copy updates to address

Copy link

Coverage Report: βœ…
Base Coverage: 79.95%
Current Coverage: 79.95%

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.

Looks good! just added a comment to keep track of our TODO items

to: '',
},
title: '',
};
Copy link
Contributor

Choose a reason for hiding this comment

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

please add TODO VM_Placement comments where needed

@bnussman-akamai bnussman-akamai added the Approved Multiple approvals and ready to merge! label Jan 22, 2024
@carrillo-erik carrillo-erik merged commit f845204 into linode:develop Jan 22, 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.

None yet

4 participants