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-7109] - Add AGLB Delete Route Dialog #9735

Conversation

bnussman-akamai
Copy link
Member

Description 📝

  • Adds a basic AGLB Route Delete Dialog

Preview 📷

Screenshot 2023-09-29 at 12 35 29 PM

How to test 🧪

  • Check out this PR
  • Turn the MSW on
  • Go to http://localhost:3000/loadbalancers/0/routes
  • Test the Delete Dialog
    • Check spelling ✏️
    • Check styles 🎨
    • Check functionality 🎮
  • Verify a DELETE https://api.linode.com/v4beta/aglb/0/routes/24 API call happens on delete
  • Verify existing routes get invalidated upon delete (You should see a GET https://api.linode.com/v4beta/aglb/0/routes?page=1&page_size=25 API call)

@bnussman-akamai bnussman-akamai added the ACLB Relating to the Akamai Cloud Load Balancer label Sep 29, 2023
@bnussman-akamai bnussman-akamai requested a review from a team as a code owner September 29, 2023 16:39
@bnussman-akamai bnussman-akamai self-assigned this Sep 29, 2023
@bnussman-akamai bnussman-akamai requested review from jaalah-akamai and abailly-akamai and removed request for a team September 29, 2023 16:39
Comment on lines +207 to +212
<DeleteRouteDialog
loadbalancerId={Number(loadbalancerId)}
onClose={() => setIsDeleteDialogOpen(false)}
open={isDeleteDialogOpen}
route={selectedRoute}
/>
Copy link
Member Author

Choose a reason for hiding this comment

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

I tried conditionally rendering like how we discussed in cafe, but the animation close breaks so I'm using our normal pattern

Copy link
Contributor

Choose a reason for hiding this comment

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

I actually do wonder if there is actual benefit in conditionally rendering the dialog, since the element isn't even mounted unless open is true. It sounds to me like performance issues would be more relevant when mounting a Dialog with a big tree (maybe like the Linode Resize dialog for instance). The docs mention a keepMounted prop to that effect, but it sounds like the opposite of what we usually want to do. From what I could read, it's a case that should be supported by performance benchmarks to justify its usage (when it outside the scope of a micro optimisation).

@mjac0bs mjac0bs self-requested a review September 29, 2023 17:23
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! (please add a changeset)

  • Copy ✅
  • Behavior ✅
  • Requests and invalidation ✅

Comment on lines +207 to +212
<DeleteRouteDialog
loadbalancerId={Number(loadbalancerId)}
onClose={() => setIsDeleteDialogOpen(false)}
open={isDeleteDialogOpen}
route={selectedRoute}
/>
Copy link
Contributor

Choose a reason for hiding this comment

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

I actually do wonder if there is actual benefit in conditionally rendering the dialog, since the element isn't even mounted unless open is true. It sounds to me like performance issues would be more relevant when mounting a Dialog with a big tree (maybe like the Linode Resize dialog for instance). The docs mention a keepMounted prop to that effect, but it sounds like the opposite of what we usually want to do. From what I could read, it's a case that should be supported by performance benchmarks to justify its usage (when it outside the scope of a micro optimisation).

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.

Confirmed copy, styling, and mock requests are all as expected! 🚢

@mjac0bs mjac0bs added Approved Multiple approvals and ready to merge! Missing Changeset labels Oct 2, 2023
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