upcoming: [M3-8021] - Manage Image Regions Drawer by bnussman-akamai · Pull Request #10617 · linode/manager

bnussman-akamai · 2024-06-26T18:07:47Z

Description 📝

This is for Image Service Gen2 ✨💾
Adds drawer for Manage Regions option on an Image's action menu 🌐

Preview 📷

How to test 🧪

Prerequisites

Turn on the MSW

Verification steps

Test the Manage Regions drawer
- Note that the whole "e2e" flow won't work perfect using the MSW

As an Author I have considered 🤔

hkhalil-akamai

Great work so far, this is shaping up nicely!

hkhalil-akamai · 2024-06-26T20:28:17Z

packages/manager/src/features/Images/ImagesLanding/ImageRegions/ManageImageRegionsDrawer.tsx

+
+export const ManageImageRegionsDrawer = (props: Props) => {
+  const { image, onClose } = props;
+  const open = image !== undefined;


Noticing a flash of content when closing the drawer since image becomes undefined -- one workaround is using usePrevious(image) like in EditImageDrawer.tsx

I think this might be addressed by bfccbd4

I updated how state is handled in ImagesLanding so that the whole Image object is no longer store in state. It is now derived directly from the React Query data. I did this because we'll want the ManageImageRegionsDrawer to show the latest statuses because we'll be polling for updates.

bnussman-akamai · 2024-06-27T17:47:47Z

packages/manager/src/features/Images/ImagesLanding/ImageRegions/ManageImageRegionsDrawer.tsx

+  useEffect(() => {
+    if (open) {
+      resetForm({ regions: imageRegionIds });
+      reset();
+    }
+  }, [open]);


Is there a better way to do this?

github-actions · 2024-06-27T18:08:26Z

Coverage Report: ✅
Base Coverage: 82.18%
Current Coverage: 82.18%

hkhalil-akamai

All functionality looks great when tested under the MSW. Thank you for adding comprehensive unit and integration tests.

✅ Opening the drawer from the regions list and actions menu
✅ Viewing existing regions and their statuses
✅ Adding new regions
✅ Saving updates (image service is not live yet)

hkhalil-akamai · 2024-06-28T13:52:39Z

packages/manager/src/features/Images/ImagesLanding/ImageRegions/ManageImageRegionsDrawer.tsx

+          <Stack spacing={1}>
+            {values.regions.length === 0 && (
+              <Typography py={1} textAlign="center">
+                No Regions Selected


Based on the UX mockups, there should always be at least one region selected -- should we disable the option to remove the region is there is only 1 left? I'm not sure how this endpoints works on the API side: can an image be moved to a completely new set of regions or does there have to be at least one previous region when replicating?

If not, can we add a unit test to ensure this message displays when no regions are selected?

should we disable the option to remove the region is there is only 1 left

We could. Currently I built it so that the form validates that >= 1 region is selected. I put this empty state here for that case and for robustness.

I think using form field validation is fine because it gives the user good context when it says you must have at least one region selected.

abailly-akamai

Implementation looks great! Now (unrelated to your work)...

I think this UI is unfortunate and quite frankly unacceptable. A user starts selecting from a multi select which populates a lit below the user can't even see.

Screen.Recording.2024-06-28.at.10.54.49.mov

This is just bad and i believe we had decided to stay away from this pattern in favor of just using the multi select, just like for multicluster access key

Screen.Recording.2024-06-28.at.11.00.50.mov

I think we need to push back on UI to go that route, stay consistent and provide better experience for our users

packages/api-v4/src/images/images.ts

packages/manager/cypress/support/intercepts/images.ts

packages/manager/src/features/Images/ImagesLanding/ImageRegions/ImageRegionRow.tsx

abailly-akamai · 2024-06-28T14:40:40Z

packages/manager/src/features/Images/ImagesLanding/ImageRegions/ManageImageRegionsDrawer.tsx

+  useEffect(() => {
+    resetForm({ regions: imageRegionIds });
+    reset();
+  }, [imageRegionIds, open]);


you should use the onExited here too

I guess that would work, but what if the regions change while the drawer is closed? It would show stale data when reopened. Not sure if that's something we should account for or not.

hmm - I see what you mean. On the other hand if we were worried that the regions would change why the drawer is closed maybe it points out that the fetching should be in the drawer?

It's not a big deal either way, I just can't fathom why there isn't an onOpen for the drawer...

reading further, onTransitionEnd would work for both opening and closing perhaps
mui/material-ui#36728

again, that useEffect seems rather inoffensive tho we've had surprises in the past

It's kind of a new pattern for us, but let me know what you think about the changes in 9d8c0d6.

It changes how the Drawer's contents are mounted and it uses react-hook-form's values option to react to the Image's regions changing

…gionsPayload` instead of `regions: string[]`

…ATUS`

bnussman-akamai · 2024-06-28T20:23:14Z

packages/manager/src/features/Images/ImagesLanding/ImagesLanding.tsx

+      <Drawer
+        onClose={() => setIsManageRegionsDrawerOpen(false)}
+        open={isManageRegionsDrawerOpen}
+        title={`Manage Regions for ${selectedImage?.label}`}
+      >
+        <ManageImageRegionsForm
+          image={selectedImage}
+          onClose={() => setIsManageRegionsDrawerOpen(false)}
+        />
+      </Drawer>


I don't love how this looks, but it gives us the benefit of not having to worry about resetting the form onOpen or onClose. ManageImageRegionsForm gets unmounted when isManageRegionsDrawerOpen is false, which resets the form and is probably better for performance in general anyway.

I'm wondering if we should be doing Drawers this way more often

I don't know how I feel about this. The pattern does not look very intuitive or developer friendly to me (esp a form taking an onClose since a form can technically be reusable outside of a drawer). The reset on a form state has the benefit of being very explicit while this solution obscures the logic

Not blocking this PR, but I am still not convinced here - functionality is good but developer experience would need to be improved if this is a pattern we'd start using more.

bnussman-akamai · 2024-07-02T13:33:02Z

@abailly-akamai

A user starts selecting from a multi select which populates a lit below the user can't even see.

Just so we're on the same page, the regions are not added to the lower list until the Multi-Select popover is closed.

I assume this don't change much, but does your UX feedback still stand?

abailly-akamai · 2024-07-02T13:52:18Z

@bnussman-akamai it does still stand since the visual end result is the same. No progress on getting UX to reconsider and provide a similar solution for what we have done for the multicluster access key?

bnussman-akamai · 2024-07-03T14:28:55Z

UX would like to continue with the current approach

abailly-akamai

Nice work! While i don't approve of the UI approach, functionality/code is solid from my vantage point.

Approving pending a couple questions ✅

abailly-akamai · 2024-07-03T17:28:18Z

packages/manager/src/features/Images/ImagesLanding/ImageRegions/ManageImageRegionsForm.tsx

+
+      enqueueSnackbar('Image regions successfully updated.', {
+        variant: 'success',
+      });


Do we mean to not close the drawer when saving regions?

abailly-akamai · 2024-07-03T17:30:48Z

packages/manager/src/features/Images/ImagesLanding/ImageRegions/ManageImageRegionsForm.tsx

+        <Notice text={errors.root.message} variant="error" />
+      )}
+      <Typography>
+        Custom images are billed monthly, at $.10/GB. Check out{' '}


Do we usually display $.10/GB rather than $0.10/GB??

my initial glance read 10/GB so wanted to make sure

Am still curious about this one

my initial glance read 10/GB so wanted to make sure

I saw this and had the same thought, so came here! We should keep $0.10/GB, as we have elsewhere.

abailly-akamai · 2024-07-03T17:34:03Z

packages/manager/src/features/Images/ImagesLanding/ImagesLanding.tsx

+      <Drawer
+        onClose={() => setIsManageRegionsDrawerOpen(false)}
+        open={isManageRegionsDrawerOpen}
+        title={`Manage Regions for ${selectedImage?.label}`}
+      >
+        <ManageImageRegionsForm
+          image={selectedImage}
+          onClose={() => setIsManageRegionsDrawerOpen(false)}
+        />
+      </Drawer>


Not blocking this PR, but I am still not convinced here - functionality is good but developer experience would need to be improved if this is a pattern we'd start using more.

abailly-akamai · 2024-07-03T17:39:31Z

I will also just leave this here for legacy 😢

bnussman added 4 commits June 26, 2024 12:56

save progress

7034282

save progress

5bb6614

save progress

0988116

begin adding unit testing

4145be3

bnussman-akamai added Work in Progress Image Service Gen2 labels Jun 26, 2024

bnussman-akamai requested a review from hkhalil-akamai June 26, 2024 18:07

bnussman-akamai self-assigned this Jun 26, 2024

add more unit testing

17e1129

hkhalil-akamai reviewed Jun 27, 2024

View reviewed changes

bnussman added 3 commits June 27, 2024 12:57

update how image is passed via props

bfccbd4

add test and adjust regions filter

a708725

dial in

1945d9d

bnussman-akamai removed the Work in Progress label Jun 27, 2024

bnussman-akamai marked this pull request as ready for review June 27, 2024 17:41

bnussman-akamai requested a review from a team as a code owner June 27, 2024 17:41

bnussman-akamai requested review from abailly-akamai and carrillo-erik and removed request for a team June 27, 2024 17:41

bnussman-akamai commented Jun 27, 2024

View reviewed changes

bnussman added 2 commits June 27, 2024 14:29

more testing

34c5e70

add cypress test

55e8b66

bnussman-akamai requested a review from a team as a code owner June 27, 2024 21:39

bnussman-akamai requested review from cliu-akamai and hkhalil-akamai and removed request for a team June 27, 2024 21:39

fix spelling

d6589c4

bnussman-akamai added the Ready for Review label Jun 27, 2024

Added changeset: Add Manage Image Regions Drawer

df01586

bnussman-akamai requested a review from jdamore-linode June 27, 2024 21:50

hkhalil-akamai approved these changes Jun 28, 2024

View reviewed changes

abailly-akamai reviewed Jun 28, 2024

View reviewed changes

bnussman added 5 commits June 28, 2024 11:54

update cypress jsdoc

25d7be0

Added changeset: Update updateImageRegions to accept `UpdateImageRe…

79a10d3

…gionsPayload` instead of `regions: string[]`

add Readonly utility type to `IMAGE_REGION_STATUS_TO_STATUS_ICON_ST…

9d8c0d6

…ATUS`

rearchitecture so that useEffect is not needed

9ea4837

update unit tests

6ff3336

bnussman-akamai commented Jun 28, 2024

View reviewed changes

Merge branch 'develop' into M3-8021-images-manage-regions

908fe16

bnussman-akamai requested a review from abailly-akamai July 3, 2024 14:28

abailly-akamai approved these changes Jul 3, 2024

View reviewed changes

bnussman-akamai merged commit c7842f0 into linode:develop Jul 3, 2024

This was referenced Jul 8, 2024

Release v1.123.0 - release → staging #10648

Merged

Release v1.123.0 - staging → master #10652

Merged

Conversation

bnussman-akamai commented Jun 26, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description 📝

Preview 📷

How to test 🧪

Prerequisites

Verification steps

As an Author I have considered 🤔

Uh oh!

hkhalil-akamai left a comment

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!

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

Uh oh!

Uh oh!

hkhalil-akamai left a comment

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!

bnussman-akamai Jul 3, 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!

abailly-akamai left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

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!

bnussman-akamai Jun 28, 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bnussman-akamai commented Jul 2, 2024

Uh oh!

abailly-akamai commented Jul 2, 2024

Uh oh!

bnussman-akamai commented Jul 3, 2024

Uh oh!

abailly-akamai left a comment

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 commented Jul 3, 2024

Uh oh!

Reviewers

Assignees

Labels

bnussman-akamai commented Jun 26, 2024 •

edited

Loading

github-actions bot commented Jun 27, 2024 •

edited

Loading

bnussman-akamai Jul 3, 2024 •

edited

Loading

bnussman-akamai Jun 28, 2024 •

edited

Loading