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

refactor: [M3-9531] - Nodebalancer routing (Tanstack) #11858

Merged
merged 16 commits into from
Mar 19, 2025

Conversation

abailly-akamai
Copy link
Contributor

@abailly-akamai abailly-akamai commented Mar 15, 2025

Description 📝

This PR contributes to the refactor of Cloud Manager Routing. It implements the Tanstack API for the Nodebalancers feature.

Changes 🔄

  • Implements tanstack routing utils
  • Route Nodebalancer Delete modal and Unassign Modal
  • Fix units and e2e tests

Preview 📷

  • Aside from the Delete & Unassign modals inheriting a loading state, there should be no visual or functional regressions as a result of this PR

How to test 🧪

Verification steps

  • Navigate to /nodebalancers:
    • compare overall behavior with production
      • Tab navigation
      • Create flow
      • Delete flow
      • Unassign Firewall flow
      • Clicking on Ports in the NB Summary sidebar
  • Navigate to Firewall > Detail and confirm add firewall and unassigmnent still works
  • Navigate to Linode > Detail > Networking and confirm add firewall and unassigmnent still works
Author Checklists

As an Author, to speed up the review process, I 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

  • I have read and considered all applicable items listed above.

As an Author, before moving this PR from Draft to Open, I confirmed ✅

  • All unit tests are passing
  • TypeScript compilation succeeded without errors
  • Code passes all linting rules

};

return <NodeBalancerConfigurations {...props} {...matchProps} />;
};
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Dealing with Class Components is a bit trickier, and the cleanest way is to wrap the class component in a functional one and pass the params as props

@abailly-akamai abailly-akamai marked this pull request as ready for review March 17, 2025 14:27
@abailly-akamai abailly-akamai requested a review from a team as a code owner March 17, 2025 14:27
@abailly-akamai abailly-akamai requested review from dwiley-akamai and bill-akamai and removed request for a team March 17, 2025 14:27
Copy link

github-actions bot commented Mar 17, 2025

Coverage Report:
Base Coverage: 79.96%
Current Coverage: 79.96%

Copy link
Contributor

@bill-akamai bill-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, @abailly-akamai with this clean migration. The route organization looks very maintainable. Confirming all verifications as well.

Copy link
Contributor

@dwiley-akamai dwiley-akamai left a comment

Choose a reason for hiding this comment

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

  • Tab navigation ⚠️

Did we lose some keyboard accessibility with transition to Tanstack Router? I was unable to switch tabs using the directional arrows on my keyboard as I'm able to do in prod.

Video
Screen.Recording.2025-03-18.at.4.51.53.PM.mov

(focus changes are from me hitting the Tab key, the rest of the time I was pressing arrows)

  • Create flow ✅
  • Delete flow ⚠️

It doesn't seem that we'd want the breadcrumb change in the background:

Prod This Branch
Screenshot 2025-03-18 at 4 44 13 PM Screenshot 2025-03-18 at 4 44 35 PM
  • Unassign Firewall flow ✅
  • Clicking on Ports in the NB Summary sidebar ✅
  • Navigate to Firewall > Detail and confirm add firewall and unassigmnent still works ✅
  • Navigate to Linode > Detail > Networking and confirm add firewall and unassigmnent still works ✅

@abailly-akamai
Copy link
Contributor Author

@dwiley-akamai fixed the breadcrumb in acce170

Great catch on the tabs 👀 - it's still using reach-tabs but somehow it indeed messed up the accessibility with navigation

I will create a separate ticket (high priority) in order not to extend the scope of this PR as I also will add a component test to avoid future regressions

Copy link
Contributor

@dwiley-akamai dwiley-akamai left a comment

Choose a reason for hiding this comment

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

Code review ✅
Verified breadcrumb fix ✅

@linode-gh-bot
Copy link
Collaborator

Cloud Manager UI test results

🎉 539 passing tests on test run #14 ↗︎

❌ Failing✅ Passing↪️ Skipped🕐 Duration
0 Failing539 Passing3 Skipped109m 26s

@abailly-akamai abailly-akamai merged commit 2d13651 into linode:develop Mar 19, 2025
25 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants