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

feat: [M3-6819] - AGLB Details - Configurations Tab #9591

Merged
merged 13 commits into from
Sep 7, 2023
Merged

feat: [M3-6819] - AGLB Details - Configurations Tab #9591

merged 13 commits into from
Sep 7, 2023

Conversation

bnussman-akamai
Copy link
Member

@bnussman-akamai bnussman-akamai commented Aug 24, 2023 •
edited
Loading

Description 📝

  • Add AGLB Configurations Tab on the AGLB details page 🎉

Features

  • Ability to add and remove certs

Features to add in follow up tickets

  • Routes Table
  • Upload Certificate Drawer

Preview 📷

Screenshot 2023-08-25 at 1 27 40 PM

How to test 🧪

  • Test basic functionality of the configurations tab
    • Saving and similar things may not work due to the nature of the MSW and React Query

@bnussman-akamai bnussman-akamai added the ACLB Relating to the Akamai Cloud Load Balancer label Aug 24, 2023
@bnussman-akamai bnussman-akamai self-assigned this Aug 24, 2023
@bnussman-akamai bnussman-akamai changed the title add aglb configs tab - basic layout feat: [M3-6819] - AGLB Details - Configurations Tab Aug 24, 2023
@bnussman-akamai bnussman-akamai marked this pull request as ready for review August 25, 2023 17:29
abailly-akamai
abailly-akamai reviewed Aug 29, 2023
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.

UI looks great! left some comments to improve things where it could.

Screenshot 2023-08-29 at 09 45 35

Screenshot 2023-08-29 at 10 01 48

...ges/manager/src/features/LoadBalancers/LoadBalancerDetail/Certificates/CertificateSelect.tsx Show resolved Hide resolved
...ger/src/features/LoadBalancers/LoadBalancerDetail/Configurations/ApplyCertificatesDrawer.tsx Show resolved Hide resolved
...ger/src/features/LoadBalancers/LoadBalancerDetail/Configurations/ApplyCertificatesDrawer.tsx Outdated Show resolved Hide resolved
...es/manager/src/features/LoadBalancers/LoadBalancerDetail/Configurations/CertificateTable.tsx Outdated Show resolved Hide resolved
...ager/src/features/LoadBalancers/LoadBalancerDetail/Configurations/ConfigurationAccordion.tsx
flexWrap="wrap"
gap={1}
justifyContent="space-between"
pr={2}
Copy link
Contributor

Choose a reason for hiding this comment

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

This is starting to look like tailwind. Would be nice to discuss standardising these styling patterns a bit

...ager/src/features/LoadBalancers/LoadBalancerDetail/Configurations/ConfigurationAccordion.tsx Show resolved Hide resolved
...ager/src/features/LoadBalancers/LoadBalancerDetail/Configurations/ConfigurationAccordion.tsx Show resolved Hide resolved
packages/manager/src/queries/aglb/configurations.ts Show resolved Hide resolved
@bnussman-akamai
Copy link
Member Author

@abailly-akamai

I think we can allow two of the same cert configs for now until we get some API validation.

For the endpoint alignment, I will make sure we dial in the alignment when we fully implement and component-ize that.

mjac0bs
mjac0bs approved these changes Sep 6, 2023
View reviewed changes
Copy link
Contributor

@mjac0bs mjac0bs left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Features to add in follow up tickets

Thank you! 😄

Left some minor nitpicky feedback but overall the adding and removing certs functionality looks good.

...ager/src/features/LoadBalancers/LoadBalancerDetail/Configurations/ConfigurationAccordion.tsx
pr={2}
>
<Stack alignItems="center" direction="row" spacing={1}>
<Typography variant="h3">{configuration.label}</Typography>
Copy link
Contributor

Choose a reason for hiding this comment

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

Minor: I know it's this way in the mocks, but the larger font size for the label seems strange to me next to the regular sized Endpoints and ID details. Maybe there's nothing to be done here, because h3 is how we normally size accordion labels, and we just don't normally display other text.

image

Copy link
Member Author

Choose a reason for hiding this comment

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

Will follow up with UX 👍🏼

...ager/src/features/LoadBalancers/LoadBalancerDetail/Configurations/ConfigurationAccordion.tsx
<Typography variant="h3">{configuration.label}</Typography>
<Typography>&mdash;</Typography>
<Typography fontSize="1rem">
Port {configuration.port} -{' '}
Copy link
Contributor

Choose a reason for hiding this comment

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

When we finalize this component in the future and align things, I wonder if we need the dash at all.

Copy link
Contributor

Choose a reason for hiding this comment

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

(Whoops, intended to comment this about the dash between Endpoints but picked the wrong line in the diff.)

Copy link
Member Author

Choose a reason for hiding this comment

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

Will follow up with UX 👍🏼

...ager/src/features/LoadBalancers/LoadBalancerDetail/Configurations/ConfigurationAccordion.tsx Outdated Show resolved Hide resolved
packages/manager/src/features/LoadBalancers/LoadBalancerDetail/LoadBalancerDetail.tsx Outdated Show resolved Hide resolved
...ager/src/features/LoadBalancers/LoadBalancerDetail/Configurations/ConfigurationAccordion.tsx Show resolved Hide resolved
@mjac0bs mjac0bs added the Add'tl Approval Needed Waiting on another approval! label Sep 6, 2023
@bnussman-akamai bnussman-akamai added Approved Multiple approvals and ready to merge! and removed Add'tl Approval Needed Waiting on another approval! Ready for Review labels Sep 7, 2023
@bnussman-akamai bnussman-akamai merged commit 5b87bf8 into linode:develop Sep 7, 2023
11 checks passed
This was referenced Sep 15, 2023
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mjac0bs mjac0bs mjac0bs approved these changes

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

@cpathipa cpathipa Awaiting requested review from cpathipa

@abailly-akamai abailly-akamai Awaiting requested review from abailly-akamai

Assignees

@bnussman-akamai bnussman-akamai

Labels
ACLB Relating to the Akamai Cloud Load Balancer Approved Multiple approvals and ready to merge!
Projects
None yet
Milestone
No milestone
Development

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