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

Add routing to admin portal #3164

Merged
merged 9 commits into from
Oct 18, 2023
Merged

Add routing to admin portal #3164

merged 9 commits into from
Oct 18, 2023

Conversation

tsatam
Copy link
Collaborator

@tsatam tsatam commented Sep 12, 2023 •
edited
Loading

Which issue this PR addresses:

Fixes ARO-3500

What this PR does / why we need it:

Changes the URL of the Admin portal based on navigation state:

  • Cluster details will be at https://<portal_url:port>/<resource_id>
  • Routing for sub-panes within the cluster details is also present as a suffix on the route (e.g. /overview).
  • pkg/portal/portal.go updated to route the above routes to the frontend's index.html

Details for an individual cluster can be routed to directly and linked to.

Test plan for issue:

  • Existing E2E tests should continue to pass
  • Added routing checks to E2E checks to validate expected route changes are made
  • Added explicit E2E tests to ensure opening the page directly to a route works as expected

Is there any documentation that needs to be updated for this PR?

N/A

Additional Notes

  • This PR adds the React Router library to the admin portal NPM dependencies in order to implement routing.
  • This PR changes the <OverviewComponent> to display cluster properties in a Fluent UI <DetailsList>, as well as changes the presentation logic to display all known properties whether they are present on the given cluster or not.
    • This was done to fix the flaky end-to-end test on this component, blocking the merge of this and many other PRs.

@tsatam tsatam marked this pull request as ready for review September 13, 2023 17:41
@tsatam tsatam marked this pull request as draft September 13, 2023 21:54
@tsatam
Copy link
Collaborator Author

tsatam commented Sep 13, 2023 •
edited
Loading

TODOs discussed during sync review:

  • Add handling for authentication redirects to preserve the route. This is necessary in order for direct links to work correctly when not previously authenticated.
  • E2E tests should validate both the setting of the expected URL in different UI states, as well as the re-population of those UI states when freshly navigated directly to their corresponding URLs.
  • (nice-to-have) Populating the contents of the search bar into a search query parameter, allowing search results to be shared

@SudoBrendan SudoBrendan added loki javascript Pull requests that update Javascript code labels Sep 14, 2023
@tsatam tsatam force-pushed the ARO-3500/add-routing-to-admin-portal branch 3 times, most recently from af7eebb to 3e79412 Compare September 15, 2023 20:54
@tsatam tsatam marked this pull request as ready for review September 15, 2023 22:35
rhamitarora
rhamitarora previously approved these changes Sep 18, 2023
View reviewed changes
@github-actions github-actions bot added the needs-rebase branch needs a rebase label Sep 19, 2023
@github-actions
Copy link

Please rebase pull request.

@tsatam tsatam force-pushed the ARO-3500/add-routing-to-admin-portal branch from 3e79412 to e6483d9 Compare September 20, 2023 12:56
@github-actions github-actions bot removed the needs-rebase branch needs a rebase label Sep 20, 2023
@tsatam tsatam force-pushed the ARO-3500/add-routing-to-admin-portal branch from e6483d9 to 0eba3e2 Compare September 20, 2023 13:06
@tsatam tsatam force-pushed the ARO-3500/add-routing-to-admin-portal branch 3 times, most recently from a9537bd to 352f355 Compare September 21, 2023 17:01
@tsatam tsatam force-pushed the ARO-3500/add-routing-to-admin-portal branch from 352f355 to 189e827 Compare September 21, 2023 17:24
@tsatam
Update E2E tests for new admin portal routing
875e50a
@tsatam
Replace OverviewComponent with new implementation
ede4ad4 
- Use FluentUI DetailsList for contents
- Always display all properties, even if value is not present
- Modify E2E test to check each individual property
@tsatam
Build frontend artifacts
8d43f56
@tsatam tsatam force-pushed the ARO-3500/add-routing-to-admin-portal branch from 189e827 to 8d43f56 Compare September 22, 2023 15:58
lranjbar
lranjbar approved these changes Sep 25, 2023
View reviewed changes
Copy link
Contributor

@lranjbar lranjbar left a comment

Choose a reason for hiding this comment

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

+1 Routing is a hard thing to bring in if not done from the start but this looks like a good step!

portal/v2/src/ClusterDetailListComponents/Overview.tsx
<Stack styles={contentStackStylesNormal}>
<Stack horizontal>
<ShimmeredDetailsList
Copy link
Contributor

Choose a reason for hiding this comment

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

+1 For bringing in premade React components from fluentui.

bennerv
bennerv approved these changes Oct 17, 2023
View reviewed changes
Copy link
Collaborator

@bennerv bennerv left a comment

Choose a reason for hiding this comment

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

looks good to me

jhoreman
jhoreman approved these changes Oct 17, 2023
View reviewed changes
Copy link
Contributor

@jhoreman jhoreman left a comment

Choose a reason for hiding this comment

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

Looks good to me thanks!

@s-amann s-amann merged commit 365413c into Azure:master Oct 18, 2023
18 checks passed
ventifus pushed a commit to ventifus/ARO-RP that referenced this pull request Feb 7, 2024
@tsatam @ventifus
Add routing to admin portal (Azure#3164)
1724f2c 
* Add React Router library

* Use React Router for search params

The existing functionality using this appears to be non-functional, but its behavior
is preserved.

* Use cluster resourceID in route for details modal

* Use URL routing to handle Cluster Details navigation

* Route all admin portal frontend subroutes to index.html

* Add handling to portal login redirect to preserve original path

* Update E2E tests for new admin portal routing

* Replace OverviewComponent with new implementation

- Use FluentUI DetailsList for contents
- Always display all properties, even if value is not present
- Modify E2E test to check each individual property

* Build frontend artifacts
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lranjbar lranjbar lranjbar approved these changes

@bennerv bennerv bennerv approved these changes

@jhoreman jhoreman jhoreman approved these changes

@s-amann s-amann s-amann approved these changes

@rhamitarora rhamitarora rhamitarora approved these changes

@jewzaam jewzaam Awaiting requested review from jewzaam jewzaam is a code owner

@hawkowl hawkowl Awaiting requested review from hawkowl hawkowl is a code owner

@rogbas rogbas Awaiting requested review from rogbas rogbas is a code owner

@petrkotas petrkotas Awaiting requested review from petrkotas petrkotas is a code owner

@jharrington22 jharrington22 Awaiting requested review from jharrington22 jharrington22 is a code owner

@cblecker cblecker Awaiting requested review from cblecker cblecker is a code owner

@facchettos facchettos Awaiting requested review from facchettos

@cadenmarchese cadenmarchese Awaiting requested review from cadenmarchese cadenmarchese is a code owner

@UlrichSchlueter UlrichSchlueter Awaiting requested review from UlrichSchlueter UlrichSchlueter is a code owner

@SudoBrendan SudoBrendan Awaiting requested review from SudoBrendan SudoBrendan is a code owner

@Shivkumar13 Shivkumar13 Awaiting requested review from Shivkumar13

@yjst2012 yjst2012 Awaiting requested review from yjst2012 yjst2012 is a code owner

@anshulvermapatel anshulvermapatel Awaiting requested review from anshulvermapatel anshulvermapatel is a code owner

@hlipsig hlipsig Awaiting requested review from hlipsig hlipsig is a code owner

Assignees
No one assigned
Labels
javascript Pull requests that update Javascript code loki ready-for-review
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@tsatam @lranjbar @bennerv @jhoreman @s-amann @rhamitarora @SudoBrendan