change: [M3-8000] - GPUv2 Plan Divider & PlanSelection cleanup by abailly-akamai · Pull Request #10407 · linode/manager

abailly-akamai · 2024-04-25T16:03:51Z

Description 📝

This PR has two separate goals:

continue the consolidation of the Linode Create & K8 Create SelectionPlan components
implement the new plan divider for GPU plans

There may be more follow up as new features build on our Plan Selection interfaces, but this refactor will help much in reducing the barrier to doing so.

Changes 🔄

Improve PlansPanel (and children) APIs
- props
- naming conventions
- reusability
Improve plans selection accessibility
Add new reusable logic to enable dividing plans into separate tables
Add new gpuv2 feature flag to control Divider for GPU plans
Improve ExtendedTypes to include props tied to plan disabled state
Consolidate Disabled Tooltip
- New Component (we have two tooltips previously)
- Copy (via util)
- Placement (both tables and cards)
Update and increase selection-plan cypress test

Target release date 🗓️

5/13/2024

Preview 📷

The major visual change resulting from this PR is the Table divider feature shown below

Desktop	Mobile

Another changes include the Card chip opacity (opacity for copy but not "Limited Development Availability" chip

Before	After

How to test 🧪

Verification steps

ℹ️ Using ALPHA (the new "Ada" plans are only returned there at the moment)
ℹ️ The new gpuv2 flag is ON in development only and I didn't add it to the dev tools as it did not seem the switching feature was important here.

In the Linode create flow, select "Newark, NJ" region
- Select the GPU tab and confirm display and functionality of the new split tables
- Confirm other plan tabs are not affected
In the Kubernetes Create Flow, confirm no regression
In the Database Create & Resize flows, confirm no regression

As an Author I have considered 🤔

Check all that apply

abailly-akamai · 2024-05-01T15:47:53Z

packages/manager/cypress/e2e/core/linodes/plan-selection.spec.ts

    // Should have one disabled plan
    // Should have tooltip for the disabled plan (more than half disabled plans in the panel, but only one plan)
-    fbtClick('High Memory');
+    cy.findByText('High Memory').click();


fbtClick is now marked as deprecated

abailly-akamai · 2024-05-01T15:48:52Z

packages/manager/src/components/SelectionCard/CardBase.tsx

+        <CardBaseHeading
+          className="cardSubheadingTitle"
+          data-qa-select-card-heading={heading}
+        >


These changes help with the styling of the disabled selection card, allowing non string items to not suffer fro the opacity change

abailly-akamai · 2024-05-01T15:55:31Z

packages/manager/src/features/Databases/DatabaseDetail/DatabaseResize/DatabaseResize.tsx

        <StyledPlansPanel
+          disableSmallerPlans={{
+            selectedDiskSize: currentPlanDisk,
+          }}


The reasons for nesting this property are:

being very explicit on what the prop achieves (before this change, selectedDiskSize was employed to disable smaller plans and it was not clear at all

Allowing more conditions to be passed as the logic needs some finesse (working with the dbaas team to sort them out - see M3-8057)

abailly-akamai · 2024-05-01T15:57:22Z

packages/manager/src/features/components/PlansPanel/PlanContainer.tsx

+        },
+      ],
+    },
+  ];


Main logic for declaring plan panel splitting. It was made to be dynamic to more items can be added to the array as we're gearing up towards more granularity in our selection plans in the next few weeks/months.

At the moment, without added API support, the only logic we split plans is to do string matching on the label, per plan type (including a way to turn this off via feature flag). Considering the slow rollout of such family plans, it has been the accepted hard coded solution on both the product and engineering side for the time being.

abailly-akamai · 2024-05-01T16:03:22Z

packages/manager/src/features/components/PlansPanel/PlanSelection.tsx

  selectedId?: string;
  selectedRegionId?: Region['id'];
  showTransfer?: boolean;
-  type: PlanSelectionType;


type was really ambiguous here, since planType usually refers to a plan "family". renamed to plan for code clarity since that's what it refers to. It has actually helped me with the refactor

abailly-akamai · 2024-05-01T16:09:30Z

packages/manager/src/features/components/PlansPanel/PlanSelection.tsx

-                  text={tooltip}
-                  tooltipPosition="right"
-                />
-              )}


Both tooltips consolidated in one - was kind of a mess

abailly-akamai · 2024-05-01T16:09:54Z

packages/manager/src/features/components/PlansPanel/PlanSelectionTable.tsx

+      </TableBody>
+    </StyledTable>
+  );
+};


new component to separate concern

abailly-akamai · 2024-05-01T16:16:36Z

packages/manager/src/features/components/PlansPanel/PlanSelection.tsx

-          data-qa-plan-row={type.formattedLabel}
-          disabled={rowIsDisabled}
-          key={type.id}
-          onClick={() => (!rowIsDisabled ? onSelect(type.id) : undefined)}


The reason for this change is to prevent applying aria-labels on non-interactive elements (more info documented in M3-8045, which will be a follow up PR) and have the interactive elements within the table row carrying that assistive information instead.

Some screen-readers don't even register aria-labels on non-interactive elements and in general are agreed to add more confusion than context. Cloud Manager in general needs work towards reducing this unwanted friction.

…lanSelection components

github-actions · 2024-05-01T16:44:13Z

Coverage Report: ✅
Base Coverage: 81.82%
Current Coverage: 81.82%

dwiley-akamai · 2024-05-02T16:44:41Z

packages/manager/src/featureFlags.ts

 }

+interface gpuV2 {
+  planDivider: boolean;


Are other properties planned for the feature flag?

There will a string change there will be hence the payload. If not, it's quite ok to have it a JSON flag anyway?

dwiley-akamai · 2024-05-02T17:43:38Z

packages/manager/src/features/components/PlansPanel/constants.ts

+export const SMALLER_PLAN_DISABLED_COPY =
+  'Resizing to smaller plans is not supported';
+export const PLAN_NOT_AVAILABLE_IN_REGION_COPY =
+  "This plan isn't available for the selected region";


Should these end with periods to be consistent with the others?

agreed - done!

dwiley-akamai · 2024-05-02T18:07:21Z

packages/manager/src/features/Kubernetes/KubernetesPlansPanel/KubernetesPlanSelection.tsx

+    planBelongsToDisabledClass,
+    planHasLimitedAvailability,
+    planIsDisabled512Gb,
+    planIsTooSmall: false,


Does disabling based on planIsTooSmall only apply to DBaaS? If so, might be worth commenting here to specify why we're setting this to false

dwiley-akamai · 2024-05-02T18:48:40Z

packages/manager/src/features/components/PlansPanel/PlanSelectionTable.tsx

+      <TableBody role="radiogroup">
+        {shouldDisplayNoRegionSelectedMessage ? (
+          <TableRowEmpty
+            colSpan={9}


We can do tableCells.length instead of hardcoding the colSpan

nice - done!

jaalah-akamai

Checked these:
✅ Linode Plans
✅ Kubernetes Plans
✅ Database Plans

The only thing I noticed was NaN for Kubernetes Version

abailly-akamai · 2024-05-03T13:24:35Z

@jaalah-akamai this NaN is only in alpha cause that Db does not return our default value

dwiley-akamai

GPU tab changes with no regressions observed on other tabs or Kubernetes or DBaaS Create flows ✅

Tests pass ✅

Code review ✅

abailly-akamai self-assigned this Apr 25, 2024

abailly-akamai force-pushed the M3-8000 branch 2 times, most recently from ffd352b to 0a71f39 Compare April 26, 2024 16:13

abailly-akamai changed the title ~~M3-8000~~ change: [M3-8000] - GPUv2 Plan Divider & PlanSelection cleanup Apr 30, 2024

abailly-akamai added 11 commits April 30, 2024 23:03

wip

128c51b

wrap up logic

4c3eedc

the big ongoing cleanup

c25095a

tooltip updates

bd611cf

cleanup

1aff1b7

fix tests

627cbb9

accessibility work

a886286

accessibility work 2

c0daf13

Improved logic

4c8021b

Tests and wrap up

e21eada

Cleanup

e5f12ad

abailly-akamai force-pushed the M3-8000 branch from 507e8be to e5f12ad Compare May 1, 2024 03:03

abailly-akamai added 2 commits May 1, 2024 09:13

Fix card chip opacity

8594f82

Naming conventions

d540831

abailly-akamai commented May 1, 2024

View reviewed changes

abailly-akamai marked this pull request as ready for review May 1, 2024 16:11

abailly-akamai requested review from a team as code owners May 1, 2024 16:11

abailly-akamai requested review from carrillo-erik, dwiley-akamai and jdamore-linode and removed request for a team May 1, 2024 16:11

abailly-akamai commented May 1, 2024

View reviewed changes

Added changeset: Implement GPUv2 Plan Divider & Cleanup/Consolidate P…

a3c1846

…lanSelection components

make sure new GPU 512GB plans are not manually disabled

b0ef649

dwiley-akamai reviewed May 2, 2024

View reviewed changes

jaalah-akamai approved these changes May 2, 2024

View reviewed changes

feedback @dwiley-akamai

e380827

abailly-akamai requested a review from dwiley-akamai May 3, 2024 13:25

dwiley-akamai approved these changes May 3, 2024

View reviewed changes

abailly-akamai merged commit 8a37043 into linode:develop May 3, 2024

abailly-akamai mentioned this pull request May 8, 2024

fix: [M3-8057] - Disable dedicated > shared DatabaseResize same plan size selection #10450

Merged

12 tasks

This was referenced May 13, 2024

Release v1.119.0 - release → staging #10457

Merged

Release v1.119.0 - staging → master #10458

Merged

Conversation

abailly-akamai commented Apr 25, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description 📝

Changes 🔄

Target release date 🗓️

Preview 📷

How to test 🧪

Verification steps

As an Author I have considered 🤔

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

abailly-akamai May 1, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented May 1, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jaalah-akamai left a comment

Choose a reason for hiding this comment

Uh oh!

abailly-akamai commented May 3, 2024

Uh oh!

dwiley-akamai left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

abailly-akamai commented Apr 25, 2024 •

edited

Loading

abailly-akamai May 1, 2024 •

edited

Loading

github-actions bot commented May 1, 2024 •

edited

Loading