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-7288] - AGLB create page with Actions buttons. #9825

Merged
merged 28 commits into from
Oct 26, 2023

Conversation

cpathipa
Copy link
Contributor

@cpathipa cpathipa commented Oct 23, 2023

Description 📝

AGLB create page with Actions buttons.

Note: Action buttons behavior, Stepper component contents are not part of this PR scope will be implemented in the followup subtasks.

Preview 📷

image

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 Create page W.R.T mockup.
  • Verify Stepper component functionality by clicking next and previous.

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 23, 2023 15:07
@cpathipa cpathipa requested review from hana-akamai and coliu-akamai and removed request for a team October 23, 2023 15:07
@cpathipa cpathipa marked this pull request as draft October 23, 2023 15:08
@cpathipa cpathipa self-assigned this Oct 23, 2023
@cpathipa cpathipa added ACLB Relating to the Akamai Cloud Load Balancer Work in Progress labels Oct 23, 2023
Comment on lines 38 to 70
{/* TODO:
* Implement Review Load Balancer Action Behavior
* Implement Cancel Behavior
* Implement Add Another Configuration Behavior
*/}
<div
style={{
display: 'flex',
justifyContent: 'space-between',
marginTop: '16px',
width: '100%',
}}
>
<div>
<StyledAddConfigurationButton
buttonType="outlined"
data-qa-api-cli-linode
onClick={() => null}
>
Add Another Configuration
</StyledAddConfigurationButton>
</div>

<ActionsPanel
primaryButtonProps={{
label: 'Review Load Balancer',
onClick: () => null,
}}
secondaryButtonProps={{ label: 'Cancel', onClick: () => null }}
sx={{ margin: 0, padding: 0 }}
/>
</div>
</Grid>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Action button behaviors will be implement in followup tickets.

Comment on lines 12 to 19
// TODO: Regious should be updated with the real values
const regions = [
{ country: 'ga', id: 'us-southeast', label: 'Atlanta, GA' },
{ country: 'ca', id: 'ca-central', label: 'Toronto, CA' },
{ country: 'au', id: 'ap-southeast', label: 'Sydney, AU' },
{ country: 'gb', id: 'eu-west', label: 'London, UK' },
{ country: 'fr', id: 'fr-par', label: 'Paris, FR' },
];
Copy link
Contributor Author

Choose a reason for hiding this comment

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

These are the hardcoded dummy regions used for the dev purpose. These are replace with correct regions once we get them.

@cpathipa cpathipa marked this pull request as ready for review October 23, 2023 21:52
Copy link
Contributor

@coliu-akamai coliu-akamai left a comment

Choose a reason for hiding this comment

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

great work Chandra! Left a few minor non-blocking comments and also had a question regarding the mockup -- should it say Global Load Balancers / Create for the title, or Loadbalancers / Create?

UX Mockup This PR
image image

Comment on lines 52 to 60
<Grid
alignItems={'center'}
container
direction="row"
justifyContent="flex-start"
key={region.id}
marginBottom={theme.spacing(1)}
spacing={2}
>
Copy link
Member

Choose a reason for hiding this comment

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

Would a Stack make more sense here?

Copy link
Contributor

@hana-akamai hana-akamai left a comment

Choose a reason for hiding this comment

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

I think the breadcrumb should be Global Load Balancers / Create instead of Loadbalancers / Create according to the mocks

This branch UX Mocks
this branch mocks

The action buttons at the bottom for mobile look a bit awkward imo, do we have mobile UX mocks?
image


describe('LoadBalancerConfiguration', () => {
test('Should render Details content', () => {
renderWithTheme(<LoadBalancerConfiguration />);
Copy link
Contributor

Choose a reason for hiding this comment

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

We can clean this up a little by doing this instead and with the other tests

    const { getByText, queryByText } = renderWithTheme(
      <LoadBalancerConfiguration />
    );

Copy link
Contributor Author

Choose a reason for hiding this comment

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

IMO, I don't see any significant benefits in choosing between using 'screen' methods
and destructuring the 'render' result. Both approaches are concise and easy to read, and I have
no strong opinions on which one to use. It largely depends on personal preference.

Copy link
Member

Choose a reason for hiding this comment

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

While screen works great, I also have a deep fear that it one day will cause issues. It scares me that it's just some magic global that happens to have the correct scope when used in a test. I have no idea how it is implemented but I generally prefer destructuring the 'render' result because it seems more direct and less "magic" to me. I'm sure either method is fine but just wanted to provide another perspective.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Had a quick look in into testing-library documentation, looks like they are recommending screen methods to find DOM elements (Didn't do in-depth research) and it also supports multiple frameworks.
image

Copy link
Member

Choose a reason for hiding this comment

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

Very interesting, good find. 🔎

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.

I think the breadcrumb should be Global Load Balancers / Create instead of Loadbalancers / Create according to the mocks

Agreed. Unless: are we keeping "Global" in the terminology, @bnussman-akamai? I don't fully remember what UX showed in the last cafe in regard to the AGLB menu options -- was "Global" still in there? Was wondering if this should be Load Balancers / Create.

cpathipa and others added 7 commits October 24, 2023 12:05
…/LoadBalancerRegions.tsx

Co-authored-by: Mariah Jacobs <114685994+mjac0bs@users.noreply.github.com>
…21284.md

Co-authored-by: Mariah Jacobs <114685994+mjac0bs@users.noreply.github.com>
…/LoadBalancerConfiguration.tsx

Co-authored-by: Connie Liu <139280159+coliu-akamai@users.noreply.github.com>
…/LoadBalancerRegions.tsx

Co-authored-by: Connie Liu <139280159+coliu-akamai@users.noreply.github.com>
@hana-akamai
Copy link
Contributor

@cpathipa can we swap the order of the cancel and action buttons for the mobile view to match the desktop version more? It also feels weird to have the cancel to the right side

Desktop Mobile
desktop mobile

@cpathipa
Copy link
Contributor Author

Thanks @hana-linode, removed the row-reverse. Now its in sync with desktop view
image

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.

Nice work, looking good and matches the mocks! 🚢

I did notice one file name typo.

cpathipa and others added 3 commits October 25, 2023 15:25
@mjac0bs mjac0bs added the Add'tl Approval Needed Waiting on another approval! label Oct 25, 2023
@bnussman-akamai
Copy link
Member

UI looks great! I think the last thing we need to do is make the stepper wrap on mobile

Screenshot 2023-10-25 at 7 55 13 PM

@cpathipa
Copy link
Contributor Author

Wrapped Stepper component for small screen.

image

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.

Nice Job!

Ui looks good ✅
Test coverage added 👍

left a small comment for using a util instead of duplicating logic

Copy link
Member

@bnussman-akamai bnussman-akamai left a comment

Choose a reason for hiding this comment

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

Awesome job! Looks great!

@bnussman-akamai bnussman-akamai added Approved Multiple approvals and ready to merge! and removed Add'tl Approval Needed Waiting on another approval! labels Oct 26, 2023
Comment on lines 11 to 15
{ country: 'us', id: 'us-iad', label: 'Washington, DC' },
{ country: 'us', id: 'us-lax', label: 'Los Angeles, CA' },
{ country: 'fr', id: 'fr-par', label: 'Paris, FR' },
{ country: 'jp', id: 'jp-osa', label: 'Osaka, JP' },
{ country: 'au', id: 'ap-southeast', label: 'Sydney, AU' },
Copy link
Contributor Author

Choose a reason for hiding this comment

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

TY @bnussman for updating with approved regions. Was about to push this change then realized your commit.

Copy link
Contributor

@coliu-akamai coliu-akamai left a comment

Choose a reason for hiding this comment

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

UI looks good + looks good on smaller screens! 🎉

@cpathipa cpathipa merged commit 9cd7af2 into linode:develop Oct 26, 2023
12 of 13 checks passed
@cpathipa cpathipa deleted the M3-7288 branch October 26, 2023 14:08
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.

7 participants