upcoming: [M3-7614] - Placement Groups Create/Rename Drawers #10106

abailly-akamai · 2024-01-24T22:13:03Z

Description 📝

This PR implements the "Create" & "Rename" Placement Groups drawers. It does a simple abstraction of its shared components to which we pass the formik props and optional placement group data.

I wanted to avoid relying on a "mode" state to have a more robust separation of concern between both drawers and avoid mashing two mutations together. It's a bit more readable and scalable that way, and doesn't part from our existing conventions.

Changes 🔄

Add Drawers, related components and their tests
Change affinity type to be an enum (usually a good practice for small, static select options)
Rename "update" nomenclature to be "rename" since this is all that drawer does (and likely will do)
Code cleanup and tightening

Preview 📷

Create Drawer	Rename Drawer

How to test 🧪

Prerequisites

Have the "Placement Group" flag and MSW turned on

Verification steps

Create Drawer
- Navigate to http://localhost:3000/placement-groups
- Click on "Create Placement Group"
- Confirm behavior, error handling, validation and submission
Rename Drawer
- Navigate to http://localhost:3000/placement-groups
- Click on "Rename" CTA for one of the three PGs in the table
- Confirm behavior, error handling, validation and submission

As an Author I have considered 🤔

Check all that apply

abailly-akamai · 2024-01-29T14:51:53Z

packages/api-v4/src/placement-groups/types.ts

  compliant: boolean;
  linode_ids: number[];
-  limits: number;
+  capacity: number;


from updated API specs

abailly-akamai · 2024-01-29T14:53:10Z

packages/manager/src/features/PlacementGroups/PlacementGroupsCreateDrawer.tsx

+} from 'src/utilities/formikErrorUtils';
+
+import { PlacementGroupsDrawerContent } from './PlacementGroupsDrawerContent';
+import { MAX_NUMBER_OF_PLACEMENT_GROUPS } from './constants';


this is eventually going to be returned by the API

abailly-akamai · 2024-01-29T14:54:33Z

packages/manager/src/features/PlacementGroups/PlacementGroupsCreateDrawer.tsx

+        });
+    },
+    validateOnBlur: false,
+    validateOnChange: isFormDirty,


this allows us to implement real time validation once the form has been submitted once and we got validation errors

This is useful, maybe turning this into a hook?

export const useFormValidateOnChange = () => { const [isFormDirty, setIsFormDirty] = React.useState<boolean>(false); const setFormDirty = (value: boolean) => { setIsFormDirty(value); }; return { isFormDirty, setFormDirty }; };

I'm open to others input. Perhaps there's a way to utilize formik.dirty in this way.

I like the hook idea! gonna add it, and be a little more specific.

dirty in our case is "has been submitted at least once"

formik.dirty happens with the fist onChange event for any of the fields (which is not what we want)

however it's got formik.submitCount which we can employ as a param for the hook

abailly-akamai · 2024-01-29T14:55:46Z

packages/manager/src/features/PlacementGroups/PlacementGroupsDetail/PlacementGroupsDetail.tsx

+  if (isLoading) {
+    return <CircleProgress />;
+  }
+


otherwise "Not Found" was showing instead of our loading state

github-actions · 2024-01-29T16:25:13Z

Coverage Report: ✅
Base Coverage: 81.21%
Current Coverage: 81.21%

jaalah-akamai · 2024-01-30T02:03:35Z

✅ Rename drawer is good
✅ Validation errors look good

I noticed a view issues with create drawer:

If I select a region and remove it, I can still submit the form
If I choose to create another, Affinity Type is auto-populated

review-vm.mp4

abailly-akamai · 2024-01-30T14:48:18Z

packages/manager/src/features/PlacementGroups/utils.ts

+    label: value,
+    value: key as CreatePlacementGroupPayload['affinity_type'],
+  })
+);


wanna cast here so value isn't just a string. a bit stricter

mjac0bs

The submission and payloads look good for creating and renaming a PG. Validation for required fields and label length works in both drawers.

✅ Creating:

✅ Renaming:

I am seeing, for the briefest of moments, the PG label show "undefined" when closing out of the Rename drawer (via Cancel or X button) without submitting the form.

Screen.Recording.2024-01-30.at.11.32.19.AM.mov

packages/manager/src/features/PlacementGroups/PlacementGroupsCreateDrawer.test.tsx

packages/manager/src/features/PlacementGroups/PlacementGroupsRenameDrawer.tsx

mjac0bs · 2024-01-30T18:55:22Z

Follow up question: what's the justification for displaying the disabled Region and Affinity Type fields in the Rename drawer, if those are not editable? That information is available in the table for reference, and it seems cleaner to only have the relevant fields in the Rename drawer; in this case, Label.

abailly-akamai · 2024-01-30T19:13:28Z

@mjac0bs

fixed the "undefined" string on close

Follow up question: what's the justification for displaying the disabled Region and Affinity Type fields in the Rename drawer, if those are not editable? That information is available in the table for reference, and it seems cleaner to only have the relevant fields in the Rename drawer; in this case, Label.

glad you asked! this has come up before and that's a valid point (and I agree, would make formik types cleaner). the reasoning behind this is to

match what was done for the VPC edit drawer
eventually add some helper tooltip to the disabled fields and add some context (in which case we may also change the CTA to "edit" and not "rename"

Frankly this is a bit in flux and I suspect this may change in the next weeks so am not too concerned about it, but I will bring it back up once we get our UX DRI back (will be am easy change)

mjac0bs

Updates look good!

As far as the Rename drawer: helpful context, thank you. I'd forgotten we'd done that with VPC, but that seems to diverge from what we've done more of in the past (ex. Linode > Storage > Rename Disk; Volume > Edit). I'd be curious what disabled tooltip text might say other than some version of "You cannot edit this" and how it might be beneficial to the user. In any case, glad you've got eyes on it and the topic has come up.

On a second pass through the code and UI, I checked disabled states and blocked network requests to verify the different error states look good. Overall, LGTM -- just found a couple more typos in tests.

packages/manager/src/features/PlacementGroups/PlacementGroupsCreateDrawer.test.tsx

packages/manager/src/features/PlacementGroups/PlacementGroupsRenameDrawer.test.tsx

mjac0bs · 2024-01-30T20:06:33Z

packages/manager/src/features/PlacementGroups/PlacementGroupsDrawerContent.tsx

+              disabled:
+                !isRenameDrawer &&
+                numberOfPlacementGroupsCreated &&
+                maxNumberOfPlacementGroups
+                  ? numberOfPlacementGroupsCreated >= maxNumberOfPlacementGroups
+                  : false,


A small ❓ : Was disabling the Create button on the landing page, rather than the Create button in the drawer, discussed? Just wondering about that extra click for the user, but maybe it's an intentional choice so that the user can view the drawer.

Fair! here's extra context. We still don't know how we are going to get the PG limit. It may be per account, it may be per DC, it may be both... (this came in today) ; to that effect I will leave this as be till we get more clarification but added a comment to move it to the parent as an option.

abailly-akamai self-assigned this Jan 24, 2024

abailly-akamai force-pushed the M3-7614 branch 3 times, most recently from af45749 to bc6604c Compare January 26, 2024 22:16

abailly-akamai changed the title ~~upcoming: [M3-7614] - Placement Groups Create Drawer~~ upcoming: [M3-7614] - Placement Groups Create/Rename Drawers Jan 28, 2024

abailly-akamai force-pushed the M3-7614 branch from 87b98bb to bf841f3 Compare January 29, 2024 14:37

abailly-akamai commented Jan 29, 2024

View reviewed changes

abailly-akamai added the Placement Groups label Jan 29, 2024

abailly-akamai marked this pull request as ready for review January 29, 2024 16:02

abailly-akamai requested a review from a team as a code owner January 29, 2024 16:02

abailly-akamai requested review from mjac0bs, jaalah-akamai and carrillo-erik and removed request for a team January 29, 2024 16:02

abailly-akamai added 14 commits January 30, 2024 09:31

Initial commit: setting up the pieces

088fe5a

Form conditionals

1a20f85

Finalize create drawer

eab376b

Save work

0d6375a

Cleanup after rebase

d754e57

Cleanup after rebase

811a76f

Save progress

2579796

Cleanup and invalidation

1652b5e

cleanup issue with deps

67204ff

Tests

1fb4815

moar covering

75f0fa2

cleanup

0b89e22

cleanup 2

c0580da

Add drawer to empty state

23ce347

Added changeset: Placement Groups Create/Rename Drawers

e9c132e

abailly-akamai force-pushed the M3-7614 branch from 4d414a7 to e9c132e Compare January 30, 2024 14:32

Replace enum

c2f9233

abailly-akamai commented Jan 30, 2024

View reviewed changes

Feedback

a95dceb

jaalah-akamai approved these changes Jan 30, 2024

View reviewed changes

jaalah-akamai added the Add'tl Approval Needed Waiting on another approval! label Jan 30, 2024

implement hook for reusability

0f69156

mjac0bs reviewed Jan 30, 2024

View reviewed changes

packages/manager/src/features/PlacementGroups/PlacementGroupsCreateDrawer.test.tsx Outdated Show resolved Hide resolved

packages/manager/src/features/PlacementGroups/PlacementGroupsRenameDrawer.tsx Outdated Show resolved Hide resolved

Feedback

2289927

mjac0bs approved these changes Jan 30, 2024

View reviewed changes

mjac0bs added Approved Multiple approvals and ready to merge! and removed Add'tl Approval Needed Waiting on another approval! labels Jan 30, 2024

abailly-akamai added 2 commits January 30, 2024 15:47

Moar Feedback

56235f1

Moooaaaar Feedback

4d94acf

abailly-akamai merged commit 3937bc7 into linode:develop Jan 30, 2024
17 of 18 checks passed

cpathipa mentioned this pull request Feb 1, 2024

Release v1.111.0 - release → staging #10136

Merged

12 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

upcoming: [M3-7614] - Placement Groups Create/Rename Drawers #10106

upcoming: [M3-7614] - Placement Groups Create/Rename Drawers #10106

abailly-akamai commented Jan 24, 2024 •

edited

Loading

abailly-akamai Jan 29, 2024

abailly-akamai Jan 29, 2024

abailly-akamai Jan 29, 2024

jaalah-akamai Jan 30, 2024

abailly-akamai Jan 30, 2024

abailly-akamai Jan 29, 2024

github-actions bot commented Jan 29, 2024 •

edited

Loading

jaalah-akamai commented Jan 30, 2024

abailly-akamai Jan 30, 2024

mjac0bs left a comment •

edited

Loading

mjac0bs commented Jan 30, 2024

abailly-akamai commented Jan 30, 2024

mjac0bs left a comment

mjac0bs Jan 30, 2024

abailly-akamai Jan 30, 2024

upcoming: [M3-7614] - Placement Groups Create/Rename Drawers #10106

upcoming: [M3-7614] - Placement Groups Create/Rename Drawers #10106

Conversation

abailly-akamai commented Jan 24, 2024 • edited Loading

Description 📝

Changes 🔄

Preview 📷

How to test 🧪

Prerequisites

Verification steps

As an Author I have considered 🤔

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Jan 29, 2024 • edited Loading

jaalah-akamai commented Jan 30, 2024

Choose a reason for hiding this comment

mjac0bs left a comment • edited Loading

Choose a reason for hiding this comment

mjac0bs commented Jan 30, 2024

abailly-akamai commented Jan 30, 2024

mjac0bs left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abailly-akamai commented Jan 24, 2024 •

edited

Loading

github-actions bot commented Jan 29, 2024 •

edited

Loading

mjac0bs left a comment •

edited

Loading