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.

Sign up for GitHub

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

Jump to bottom

upcoming: [M3-7876] - Linode Create Refactor - Clone #10421

Merged
merged 18 commits into from
May 3, 2024
Merged

upcoming: [M3-7876] - Linode Create Refactor - Clone #10421

merged 18 commits into from
May 3, 2024

Conversation

bnussman-akamai
Copy link
Member

@bnussman-akamai bnussman-akamai commented Apr 30, 2024 •
edited
Loading

Description 📝

  • Adds the ability to Clone in the new Linode Create flow ✨
  • Improves validation when submitting form with no backup / linode selected

Preview 📷

Screenshot 2024-04-30 at 10 33 24 AM

How to test 🧪

Prerequisites

  • Enable the Linode Create v2 feature flag

Verification steps

  • Check the "Clone Linode" tab for parity with the existing create flow 🔍

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

@bnussman-akamai bnussman-akamai self-assigned this Apr 30, 2024
@bnussman-akamai bnussman-akamai marked this pull request as ready for review April 30, 2024 17:07
@bnussman-akamai bnussman-akamai requested a review from a team as a code owner April 30, 2024 17:07
@bnussman-akamai bnussman-akamai requested review from cpathipa, hkhalil-akamai and abailly-akamai and removed request for a team April 30, 2024 17:07
@github-actions GitHub Actions
Copy link

github-actions bot commented Apr 30, 2024 •
edited
Loading

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

bnussman-akamai
bnussman-akamai commented Apr 30, 2024
View reviewed changes
packages/manager/src/features/Linodes/LinodeCreatev2/resolvers.ts Outdated
Copy link
Member Author

Choose a reason for hiding this comment

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

There will probably be ways to consolidate and reduce duplication here, but I'm going to keep it explicit like this for now. As I become more familiar with react-hook-form, I'm sure I'll discover better ways to do things regarding our payload transformations and validators.

hkhalil-akamai
hkhalil-akamai approved these changes May 1, 2024
View reviewed changes
Copy link
Contributor

@hkhalil-akamai hkhalil-akamai left a comment

Choose a reason for hiding this comment

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

Nice work on this and great job sharing components where possible. Left some minor feedback but otherwise, great work!

✅ Code review
✅ Manual testing
✅ CI passing

packages/manager/src/features/Linodes/LinodeCreatev2/Tabs/Backups/LinodeSelectTable.tsx Outdated Show resolved Hide resolved
packages/manager/src/features/Linodes/LinodeCreatev2/Tabs/Backups/LinodeSelectTable.tsx Outdated Show resolved Hide resolved
packages/manager/src/features/Linodes/LinodeCreatev2/Tabs/Backups/LinodeSelectTableRow.tsx Outdated Show resolved Hide resolved
abailly-akamai
abailly-akamai reviewed May 2, 2024
View reviewed changes
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 great! test flow worked as expected and did not find any functionality regression.

packages/manager/src/features/Linodes/LinodeCreatev2/Tabs/Backups/LinodeSelectTable.tsx
isOpen={Boolean(linodeToPowerOff)}
linodeId={linodeToPowerOff?.id}
onClose={() => setLinodeToPowerOff(undefined)}
/>
Copy link
Contributor

@abailly-akamai abailly-akamai May 2, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I found this visual regression with the power off button rendering. I think there should be a default column with set width at all times to account for when the buttons renders?

also adding a couple issues with the way things are currently

new defect:

Screen.Recording.2024-05-02.at.09.39.23.mov

already in prod (but we could fix here)
we should use white-space: nowrap; on the button
Screenshot 2024-05-02 at 09 42 20

we don't have reboot for the cards. Is is by design?
Screenshot 2024-05-02 at 09 42 42

Copy link
Member Author

Choose a reason for hiding this comment

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

@hkhalil-akamai Any context on no "Power Off" in card view?

Copy link
Member Author

Choose a reason for hiding this comment

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

The "Power Off" button wrapping / column width issues should be fixed in 7f27bad (cc @hkhalil-akamai )

@mjac0bs mjac0bs added Approved Multiple approvals and ready to merge! and removed Ready for Review labels May 2, 2024
@bnussman-akamai bnussman-akamai merged commit aff8f48 into linode:develop May 3, 2024
18 checks passed
This was referenced May 13, 2024
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@abailly-akamai abailly-akamai abailly-akamai left review comments

@cpathipa cpathipa cpathipa approved these changes

@hkhalil-akamai hkhalil-akamai hkhalil-akamai approved these changes

@jaalah-akamai jaalah-akamai Awaiting requested review from jaalah-akamai

Assignees

@bnussman-akamai bnussman-akamai

Labels
Approved Multiple approvals and ready to merge! Linode Create Refactor
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@bnussman-akamai @cpathipa @hkhalil-akamai @abailly-akamai @mjac0bs @bnussman