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-7878] - Linode Create UI Refactor - Part 3 - Details Section #10297

Merged
merged 10 commits into from
Mar 22, 2024
Merged

upcoming: [M3-7878] - Linode Create UI Refactor - Part 3 - Details Section #10297

merged 10 commits into from
Mar 22, 2024

Conversation

bnussman-akamai
Copy link
Contributor

@bnussman-akamai bnussman-akamai commented Mar 19, 2024 •
edited

Description 📝

  • Adds the details section to the new Linode Create flow 🎉
  • Adds the following new functionality ➕
    • Linode Label Input
    • Tags Selection
    • Placement Group Selection

Preview 📷

Before After
Screenshot 2024-03-19 at 3 01 31 PM Screenshot 2024-03-19 at 3 01 59 PM

How to test 🧪

  • Turn on the Linode Create v2 feature flag
  • Test the details section
    • You should be able to input a Linode Label
    • You should be able to select tags
    • You should be able to use placements groups (use dev or the MSW)
  • Check for basic field level error handling (for example, type in a super long Linode label)
  • Check the code for style, perf, and quality

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 Mar 19, 2024
@bnussman-akamai bnussman-akamai marked this pull request as ready for review March 21, 2024 18:41
@bnussman-akamai bnussman-akamai requested a review from a team as a code owner March 21, 2024 18:41
@bnussman-akamai bnussman-akamai requested review from dwiley-akamai and hana-linode and removed request for a team March 21, 2024 18:41
@github-actions GitHub Actions
Copy link

github-actions bot commented Mar 21, 2024 •
edited

Coverage Report: ✅
Base Coverage: 81.63%
Current Coverage: 81.64%

bnussman-akamai
bnussman-akamai commented Mar 21, 2024
View reviewed changes
packages/manager/src/features/Linodes/LinodeCreatev2/Details/PlacementGroupPanel.tsx
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I broke this out into its own component so that it can subscribe to region state changes without causing the other fields (label and tags) to re-render. This is probably an over optimization, but I think it's still acceptable and clean. Open to feedback on this

Copy link
Contributor

Choose a reason for hiding this comment

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

Understood - this may warrant component naming changes down the line but totally fine for now 👍

packages/manager/src/features/Linodes/LinodeCreatev2/Details/Details.tsx
control={control}
name="tags"
/>
{showPlacementGroups && <PlacementGroupPanel />}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I intentionally separated PlacementGroupPanel out. Refer to my other comment

packages/manager/src/features/Linodes/LinodeCreatev2/Plan.tsx
Comment on lines -15 to -17
const { control } = useFormContext<CreateLinodeRequest>();

const regionId = useWatch({ control, name: 'region' });
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oops!

You don't need to use useFormContext for this. useWatch will automatically use the context

@bnussman-akamai
Copy link
Contributor Author

@abailly-akamai Placement groups should be hooked up, but I haven't been able to test it with dev probably because of the API changes? Let me know if I need to adjust anything!

jaalah-akamai
jaalah-akamai approved these changes Mar 21, 2024
View reviewed changes
Copy link
Contributor

@jaalah-akamai jaalah-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 with this, so easy to follow now

packages/manager/src/features/Linodes/LinodeCreatev2/index.tsx
@@ -29,6 +30,7 @@ export const LinodeCreatev2 = () => {
const { mutateAsync: createLinode } = useCreateLinodeMutation();

const onSubmit: SubmitHandler<CreateLinodeRequest> = async (data) => {
alert(JSON.stringify(data, null, 2));
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm guessing we're keeping this until the refactor is done, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I figured it wouldn't hurt to leave it in. It's helpful to see what payload is being sent.

It will 100% be removed before we get close to shipping this.

@bnussman-akamai bnussman-akamai added the Add'tl Approval Needed Waiting on another approval! label Mar 21, 2024
abailly-akamai
abailly-akamai reviewed Mar 22, 2024
View reviewed changes
packages/manager/src/features/Linodes/LinodeCreatev2/Details/PlacementGroupPanel.tsx
Copy link
Contributor

Choose a reason for hiding this comment

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

Understood - this may warrant component naming changes down the line but totally fine for now 👍

packages/manager/src/features/Linodes/LinodeCreatev2/Details/PlacementGroupPanel.test.tsx Show resolved Hide resolved
packages/manager/src/features/Linodes/LinodeCreatev2/Details/PlacementGroupPanel.tsx Show resolved Hide resolved
abailly-akamai
abailly-akamai approved these changes Mar 22, 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.

Carry on! looks good - thanks for the consideration for the placement group component and the test 👍

@bnussman-akamai bnussman-akamai added Approved Multiple approvals and ready to merge! and removed Add'tl Approval Needed Waiting on another approval! labels Mar 22, 2024
@bnussman-akamai bnussman-akamai merged commit 46ca9e1 into linode:develop Mar 22, 2024
18 checks passed
@hkhalil-akamai
Copy link
Contributor

@bnussman-akamai
Coming late to this but I just noticed we are no longer autofilling the label field based on the distro/region. Just confirming this was an intentional change?

@abailly-akamai
Copy link
Contributor

I've noticed that as well and forgot to comment on the latest PR. We do need to add this back

This was referenced Mar 28, 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 approved these changes

@jaalah-akamai jaalah-akamai jaalah-akamai approved these changes

@dwiley-akamai dwiley-akamai Awaiting requested review from dwiley-akamai dwiley-akamai is a code owner automatically assigned from linode/frontend

@hana-linode hana-linode Awaiting requested review from hana-linode hana-linode is a code owner automatically assigned from linode/frontend

@hkhalil-akamai hkhalil-akamai Awaiting requested review from hkhalil-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.

None yet
5 participants
@bnussman-akamai @hkhalil-akamai @abailly-akamai @jaalah-akamai @bnussman