upcoming: [M3-7875] - Linode Create Refactor - Backups by bnussman-akamai · Pull Request #10404 · linode/manager

bnussman-akamai · 2024-04-24T20:52:54Z

Description 📝

Linode Create Refactor for creating from a backup 💾

Preview 📷

How to test 🧪

Prerequisites

Turn on the Linode Create v2 feature flag

Verification steps

Check for parity with the existing Linode Create flow backups tab
Verify you can deep link to a pre-selected backup from the Linode Details page
Verify existing tabs still work as expected
Verify the page feel quick and snappy (no lag when making selections)

Note

The API does not support filtering on backups.enabled, so for now, the table will show all Linodes rather than just Linodes with backups enabled. I will work with the API team to get this sorted!

Note

I moved the position of the warning notice. I think the change is tasteful, acceptable, and in-line with our Notice patterns. If anyone has concerns about moving it I can move it to match production.

As an Author I have considered 🤔

github-actions · 2024-04-25T19:54:43Z

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

abailly-akamai

Looks great! Patterns are solid and consistent

Added a few comments before taking a second pass

abailly-akamai · 2024-04-26T13:24:51Z

packages/manager/src/features/Linodes/LinodeCreatev2/Tabs/Backups/BackupSelect.tsx

+  );
+
+  const hasNoBackups =
+    !isFetching && data?.automatic.length === 0 && !data.snapshot.current;


I always wonder if there is a risk of API data used in computation being stale right after the re-render in cases like this.

Do you mean something like this component showing backups for the previously selected Linode? Or the React Query cache being out of date containing stale data?

Sorry not being very clear. It's frankly something I am always struggling a bit with (and this example is probably just fine). This type of computation in the react world has always been advocated to be done in useEffects cause we can control rendering cycles a bit more granularly. While very happy to move away from using those, I sometimes wonder if (in this case for instance), hasNoBackups gets recalculated properly, when needed.

In this case, I think we're good. hasNoBackups should always get derived directly from the React Query state on each and every render.

Side note: I removed the isFetching from the condition. It wasn't needed based on how I did the conditional rendering.

abailly-akamai · 2024-04-26T13:37:54Z

packages/manager/src/features/Linodes/LinodeCreatev2/Tabs/Backups/LinodeSelectTable.tsx

+                />
+              ))}
+            </TableBody>
+          </Table>


You had previously dismissed a comment about the handling of the radio/label table column in the Stackscripts create flow and it would be nice for those to be consistent (they aren't in Prod either). we have a mix of floating left, extra cell, extra spacing etc

Am in favor of floating everything left without an extra column for the radio but whatever is consistent works for me in the end.

Stackscript Backups

I created M3-8041 in the epic so we can follow up, find the best pattern, and implement it on all tables

packages/manager/src/features/Linodes/LinodeCreatev2/Tabs/Backups/BackupsWarning.tsx

packages/manager/src/features/Linodes/LinodeCreatev2/utilities.ts

abailly-akamai · 2024-04-26T13:56:34Z

packages/manager/src/features/Linodes/LinodeCreatev2/utilities.ts

+  // Always default debian for the distributions tab.
+  if (!params.type || params.type === 'Distributions') {
+    return 'linode/debian11';
+  }


this is probably fine, but could be made as a constant since you are using it down on line 306

nit: I would also argue that we should check that it exists and if not select the first in array via a util. I know it's unlikely we will remove it but it's a small safety net

Yeah, handling default values has been hard. We can't really "check that it exists" at this point because we don't have images loaded into memory yet. Hoping to find better way to do it and clean up over time. Will make a constant for Debian 11 for now

Sounds good. I imagine we will want to set defaults via custom hooks so we can hook in the data in memory?

…container

SanPilot

Manual testing and code review both look good. Left some feedback including one possible bug.

packages/manager/src/features/Linodes/LinodeCreatev2/utilities.ts

SanPilot · 2024-04-26T22:09:18Z

packages/manager/src/features/Linodes/LinodeCreatev2/Tabs/Backups/LinodeSelectTable.tsx

Why are we no longer using a common selection table component for backups and clone?

I wen't ahead and created a new table because this new one is API filtered and ordered. Also, creating a new table allows us to use react-hook-form hooks closer to where they are consumed (less props required). I think these reasons warrant new code but I'm happy to reevaluate if you think I should be using more code from the old flow

packages/manager/src/features/Linodes/LinodeCreatev2/Tabs/Backups/LinodeSelectTable.tsx

SanPilot · 2024-04-26T22:17:17Z

packages/manager/src/features/Linodes/LinodeCreatev2/Tabs/Backups/LinodeSelect.tsx

+    <Paper>
+      <Stack spacing={1}>
+        <Typography variant="h2">Select Linode</Typography>
+        <BackupsWarning />


I'm not opposed to moving the notice below the header but I think UX should be informed of the change

SanPilot · 2024-04-26T22:19:56Z

packages/manager/src/features/Linodes/LinodeCreatev2/index.tsx

              <SafeTabPanel index={3}>
                <Images />
              </SafeTabPanel>
-              <SafeTabPanel index={4}>Bckups</SafeTabPanel>


Finally, this typo has been bugging me 😂

hkhalil-akamai

Whoops, signed into the wrong account 😅

Approved pending feedback from the previous review

hkhalil-akamai · 2024-04-26T21:07:21Z

packages/manager/src/features/Linodes/LinodeCreatev2/Tabs/Backups/BackupsWarning.tsx

+  return (
+    <Notice variant="warning">
+      <List sx={{ listStyleType: 'disc', pl: 2.5 }}>
+        <ListItem sx={{ display: 'list-item', pl: 1, py: '4px' }}>


Would this work?

Suggested change

<ListItem sx={{ display: 'list-item', pl: 1, py: '4px' }}>

<ListItem sx={{ display: 'list-item', pl: 1, py: 4}}>

Yeah! I updated the value to 0.5 instead of 4px. I believe MUI takes ints and multiples them by theme.spacing

abailly-akamai

Approving pending addressing @hkhalil-akamai's suggestions and comments.

Also, agree a shared Linode table for Backups/Clone would be nice if easily doable

abailly-akamai · 2024-04-29T13:11:26Z

packages/manager/src/features/Linodes/LinodeCreatev2/Tabs/Backups/BackupsWarning.tsx

+          This newly created Linode will be created with the same password and
+          SSH Keys (if any) as the original Linode.
+        </ListItem>
+        <ListItem sx={{ display: 'list-item', pl: 1, py: '4px' }}>


same here I agree using ints is always better for spacing values when possible

bnussman added 3 commits April 24, 2024 14:28

initial backups ui

f01d555

more work

f413cfb

more work

82d506c

bnussman-akamai added the Linode Create Refactor label Apr 24, 2024

bnussman-akamai self-assigned this Apr 24, 2024

bnussman added 16 commits April 24, 2024 18:05

more work and start adding testing

36f8e57

allow backup deep link

20d616a

Merge branch 'develop' into M3-7875-linode-create-refactor-backups

6080c9d

make behavior match existing flow

5d38ffe

more progress

2669123

add no Linode selected message

0c48c6f

unit test, fix bugs, and improve parity

f3fd266

make styles match existing flow

d517d23

hide region panel for backups

ee7857d

clean up and filter table on preselection

ad12bfa

move the selected linode to form state (i hope i don't regret this)

71461dd

improve glitchy rendering

e6e232b

fix breaking changes

578910a

add changesets

a58cb62

add comment

f3dc8e3

improve unit testing

9eaedec

bnussman-akamai marked this pull request as ready for review April 25, 2024 17:24

bnussman-akamai requested a review from a team as a code owner April 25, 2024 17:24

bnussman-akamai requested review from carrillo-erik and jdamore-linode and removed request for a team April 25, 2024 17:24

bnussman-akamai added the Ready for Review label Apr 25, 2024

bnussman-akamai requested review from hkhalil-akamai and removed request for carrillo-erik and jdamore-linode April 25, 2024 17:24

bnussman-akamai requested a review from abailly-akamai April 25, 2024 17:24

bnussman added 2 commits April 25, 2024 15:31

clean up and fixes

8a7bc85

Merge branch 'develop' into M3-7875-linode-create-refactor-backups

d236e91

abailly-akamai reviewed Apr 26, 2024

View reviewed changes

bnussman added 6 commits April 26, 2024 10:38

use an MUI <List />

7bab067

fix type and use constant for the default distribution

1c42e45

clean up no backups logic

6a5a666

make date time format match existing flow

bbb41c7

make grid margin match existing flow by wrapping grids in a non-flex …

7fea286

…container

Merge branch 'develop' into M3-7875-linode-create-refactor-backups

fb60e64

bnussman-akamai requested a review from abailly-akamai April 26, 2024 22:06

SanPilot approved these changes Apr 26, 2024

View reviewed changes

hkhalil-akamai approved these changes Apr 26, 2024

View reviewed changes

abailly-akamai approved these changes Apr 29, 2024

View reviewed changes

feedback from @hkhalil-akamai

bf35206

bnussman-akamai merged commit 0f593e0 into linode:develop Apr 29, 2024

This was referenced May 13, 2024

Release v1.119.0 - release → staging #10457

Merged

Release v1.119.0 - staging → master #10458

Merged

	<ListItem sx={{ display: 'list-item', pl: 1, py: '4px' }}>
	<ListItem sx={{ display: 'list-item', pl: 1, py: 4}}>

Conversation

bnussman-akamai commented Apr 24, 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!

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

Uh oh!

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!

bnussman-akamai Apr 26, 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!

Uh oh!

Uh oh!

abailly-akamai Apr 26, 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!

bnussman-akamai Apr 26, 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!

SanPilot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

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!

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!

abailly-akamai left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

bnussman-akamai commented Apr 24, 2024 •

edited

Loading

github-actions bot commented Apr 25, 2024 •

edited

Loading

bnussman-akamai Apr 26, 2024 •

edited

Loading

abailly-akamai Apr 26, 2024 •

edited

Loading

bnussman-akamai Apr 26, 2024 •

edited

Loading