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: [M3-8834] - Display new Accelerated plans in Plans Panel #11287

Merged
merged 22 commits into from
Nov 25, 2024

Conversation

coliu-akamai
Copy link
Contributor

@coliu-akamai coliu-akamai commented Nov 19, 2024

Description 📝

Adds Accelerated plans to Plans Panel

Changes 🔄

  • Introducing a feature flag, acceleratedPlans + hide everything behind feature flag (and account capability)
  • Update Plans Panel to match figma mocks (see internal ticket) for Accelerated plans
  • Update mock regions data to include accelerated capability for some regions

Target release date 🗓️

12/10

Preview 📷

How to test 🧪

Either use the MSW, or make sure you have the correct customer tag (reach out for it)

  • Confirm UI matches figma mockup for Linode plans (see internal ticket)
  • Confirm Accelerated plans are hidden when feature flag is false (inside useFlags, test when different variations of { linodePlans: boolean; lkePlans: boolean } are returned)
  • yarn test PlansPanel/utils.test.ts
  • NOTE: there are still a few open questions (doc links) - created M3-8931 as a sanity check for everything. will update this branch as I get the answers tho)

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

  • I have read and considered all applicable items listed above.

As an Author, before moving this PR from Draft to Open, I confirmed ✅

  • All unit tests are passing
    • databaseResize having some flake
  • TypeScript compilation succeeded without errors
  • Code passes all linting rules

@coliu-akamai coliu-akamai self-assigned this Nov 19, 2024
Comment on lines +403 to +405
const premiumTypes = linodeTypeFactory.buildList(7, {
class: 'premium',
});
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added these plans bc without them, we'd have a weird case where when using MSW, clicking on a plan in the Accelerated tab would make the plans panel jump to the Premium tab.

There were no (mock) premium plans, but the Premium tab still shows up (with the disabled 512gb plan). No (mock) premium plans + Premium tab still appearing = tabOrder in determineInitialPlanCategoryTab is one tab short, making the initialTab value in PlansPanel one less than it should be.

Wondering if we should prevent a tab from showing up if the only plan in that tab is the disabled 512gb plan? However, it's probably a non-issue on prod because there are Premium plans

interface PlanSelectionTableProps {
filterOptions?: PlanSelectionFilterOptionsTable;
planFilter?: (plan: PlanWithAvailability) => boolean;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

(prop isn't used)

Comment on lines -16 to -19
interface PlanSelectionFilterOptionsTable {
header?: string;
planFilter?: (plan: PlanWithAvailability) => boolean;
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

same type as PlanSelectionDividerTable from PlanContainer, so I consolidated the two. Decided to keep PlanSelectionFilterOptionsTable as the name bc we use filterOptions a lot, but can change back

@@ -69,15 +81,6 @@ export const PlanContainer = (props: PlanContainerProps) => {
!flags.dbaasV2?.beta &&
flags.dbaasV2?.enabled &&
(isDatabaseCreateFlow || isDatabaseResizeFlow);
interface PlanSelectionDividerTable {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

see comment in PlanSelectionTable

@@ -154,7 +154,7 @@ export const PlanSelectionTable = (props: PlanSelectionTableProps) => {
message={PLAN_SELECTION_NO_REGION_SELECTED_MESSAGE}
/>
) : (
renderPlanSelection(filterOptions)
renderPlanSelection()
Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed filterOptions (* unless keeping it here might be helpful down the line?) bc we'd always be passing undefined here:

  • for GPU plans, we pass in () => renderPlanSelection({...some filter}) as our renderPlanSelection prop, so the filterOptions that we also passed in { header: table.header } don't get used here
  • for non GPU plans, we don't pass in any filterOptions prop, so that becomes undefined here

@coliu-akamai coliu-akamai marked this pull request as ready for review November 20, 2024 16:25
@coliu-akamai coliu-akamai requested a review from a team as a code owner November 20, 2024 16:25
@coliu-akamai coliu-akamai requested review from bnussman-akamai, harsh-akamai and abailly-akamai and removed request for a team November 20, 2024 16:25
Copy link

github-actions bot commented Nov 20, 2024

Coverage Report:
Base Coverage: 86.96%
Current Coverage: 86.95%

@coliu-akamai coliu-akamai changed the title feat: [M3-8834] - Display new VPU plans in Plans Panel feat: [M3-8834] - Display new Accelerated plans in Plans Panel Nov 20, 2024
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! I'll do a deeper review, but leaving some early comments

coliu-akamai and others added 2 commits November 21, 2024 10:48
…ation.tsx

Co-authored-by: Alban Bailly <130582365+abailly-akamai@users.noreply.github.com>
@coliu-akamai coliu-akamai added Add'tl Approval Needed Waiting on another approval! and removed Ready for Review labels Nov 22, 2024
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.

Solid! thanks for addressing change sand the attention to details. Confirme new plans display and banner consistentcy

@harsh-akamai harsh-akamai added Approved Multiple approvals and ready to merge! and removed Add'tl Approval Needed Waiting on another approval! labels Nov 25, 2024
@linode-gh-bot
Copy link
Collaborator

Cloud Manager UI test results

🎉 455 passing tests on test run #21 ↗︎

❌ Failing✅ Passing↪️ Skipped🕐 Duration
0 Failing455 Passing2 Skipped113m 31s

@coliu-akamai coliu-akamai merged commit 1b0733f into linode:develop Nov 25, 2024
23 checks passed
Copy link

cypress bot commented Nov 25, 2024

Cloud Manager E2E    Run #6880

Run Properties:  status check passed Passed #6880  •  git commit 1b0733f185: feat: [M3-8834] - Display new Accelerated plans in Plans Panel (#11287)
Project Cloud Manager E2E
Branch Review develop
Run status status check passed Passed #6880
Run duration 28m 13s
Commit git commit 1b0733f185: feat: [M3-8834] - Display new Accelerated plans in Plans Panel (#11287)
Committer Connie Liu
View all properties for this run ↗︎

Test results
Tests that failed  Failures 0
Tests that were flaky  Flaky 1
Tests that did not run due to a developer annotating a test with .skip  Pending 2
Tests that did not run due to a failure in a mocha hook  Skipped 0
Tests that passed  Passing 455
View all changes introduced in this branch ↗︎

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!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants