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.

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

feat: [M3-7311] - Create Load Balancer flow - manage state #9848

Merged
merged 21 commits into from
Nov 2, 2023

Conversation

cpathipa
Copy link
Contributor

@cpathipa cpathipa commented Oct 30, 2023

Description 📝

Create Load Balancer flow - Manage state.

How to test 🧪

Prerequisites

(How to setup test environment)

  • Checkout the branch and run the app locally
  • Enable MSW

Verification steps

(How to verify changes)

  • Navigate to http://localhost:3000/loadbalancers
  • Click on "Create Load Balancer" Button.
  • Verify Load Balancer label field state.
  • Verify Create LoadBalancer payload schema W.R.T API spec.
  • Verify tests for LoadBalancerLabel.test.tsx.

As an Author I have considered 🤔

Check all that apply

  • 👀 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

@cpathipa cpathipa requested a review from a team as a code owner October 30, 2023 13:42
@cpathipa cpathipa requested review from jdamore-linode and coliu-akamai and removed request for a team October 30, 2023 13:42
@cpathipa cpathipa marked this pull request as draft October 30, 2023 13:42
Comment on lines -30 to -37
<LoadBalancerLabel
labelFieldProps={{
disabled: false,
errorText: '',
label: 'Linode Label',
onChange: () => null,
value: '',
}}
Copy link
Contributor Author

@cpathipa cpathipa Oct 30, 2023

Choose a reason for hiding this comment

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

Prop drilling is not required since we use a custom state management hook useLoadBalancerState useFormikContext.

@cpathipa cpathipa changed the title feat: [M3-7311] - Create Load Balancer flow - Mange state. feat: [M3-7311] - Create Load Balancer flow - Manage state. Oct 30, 2023
@cpathipa cpathipa marked this pull request as ready for review October 30, 2023 14:21
@cpathipa cpathipa self-assigned this Oct 30, 2023
@cpathipa cpathipa added the ACLB Relating to the Akamai Cloud Load Balancer label Oct 30, 2023
@mjac0bs mjac0bs changed the title feat: [M3-7311] - Create Load Balancer flow - Manage state. feat: [M3-7311] - Create Load Balancer flow - manage state Oct 30, 2023
cpathipa and others added 2 commits October 30, 2023 14:45
…78462.md

Co-authored-by: Mariah Jacobs <114685994+mjac0bs@users.noreply.github.com>
@cpathipa cpathipa added the Add'tl Approval Needed Waiting on another approval! label Nov 1, 2023
Copy link
Contributor

Choose a reason for hiding this comment

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

Unrelated linter change that snuck in.

packages/validation/src/loadbalancers.schema.ts Outdated Show resolved Hide resolved
packages/validation/src/loadbalancers.schema.ts Outdated Show resolved Hide resolved
@mjac0bs
Copy link
Contributor

mjac0bs commented Nov 1, 2023

Screenshot 2023-11-01 at 10 57 18 AM
I'm also encountering an unexpected validation error on the label field when I enter something valid and hit the Review Load Balancer button. Are you seeing the same?

cpathipa and others added 3 commits November 1, 2023 15:58
Co-authored-by: Mariah Jacobs <114685994+mjac0bs@users.noreply.github.com>
Co-authored-by: jdamore-linode <97627410+jdamore-linode@users.noreply.github.com>
@cpathipa
Copy link
Contributor Author

cpathipa commented Nov 1, 2023

@mjac0bs This issue is fixed now.

AGLB-statemanagement.mov

@cpathipa
Copy link
Contributor Author

cpathipa commented Nov 1, 2023

@mjac0bs Updated the endpointSchema as per the API spec (a4dcb76).

Copy link
Contributor

@mjac0bs mjac0bs left a comment

Choose a reason for hiding this comment

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

Thanks for fixing the label validation!

There's an issue with the endpoint validation that I commented on and an inconsistency in the API spec we'll want to double check.

My other suggestions are optional and just to use a consistent naming scheme for all our loadbalancer schemas.

packages/validation/src/loadbalancers.schema.ts Outdated Show resolved Hide resolved
packages/validation/src/loadbalancers.schema.ts Outdated Show resolved Hide resolved
packages/validation/src/loadbalancers.schema.ts Outdated Show resolved Hide resolved
packages/validation/src/loadbalancers.schema.ts Outdated Show resolved Hide resolved
packages/validation/src/loadbalancers.schema.ts Outdated Show resolved Hide resolved
packages/validation/src/loadbalancers.schema.ts Outdated Show resolved Hide resolved
packages/validation/src/loadbalancers.schema.ts Outdated Show resolved Hide resolved
@cpathipa cpathipa requested a review from mjac0bs November 2, 2023 15:52
Copy link
Contributor

@mjac0bs mjac0bs left a comment

Choose a reason for hiding this comment

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

Thanks for following up with API!

Tests and validation looks good now with the resolved dependency. I think we're just missing a changeset for the validation package.

@mjac0bs mjac0bs added Approved Multiple approvals and ready to merge! and removed Add'tl Approval Needed Waiting on another approval! labels Nov 2, 2023
@cpathipa cpathipa merged commit 3ea7090 into linode:develop Nov 2, 2023
11 checks passed
@cpathipa cpathipa deleted the M3-7311 branch November 2, 2023 17:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ACLB Relating to the Akamai Cloud Load Balancer Approved Multiple approvals and ready to merge!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants