Skip to content

improvement/navigation-core#98

Merged
IAmKio merged 4 commits intostagingfrom
improvement/navigation-core
Jul 4, 2024
Merged

improvement/navigation-core#98
IAmKio merged 4 commits intostagingfrom
improvement/navigation-core

Conversation

@IAmKio
Copy link
Collaborator

@IAmKio IAmKio commented Jul 3, 2024

Description

  • Changed react-router-dom from the BrowserRouter to createbrowserrouter which uses the new Data APIs
  • Moved the authorised component into it's own file
  • Added new catch-all / splat routes to each app to allow local app-level routing

How Has This Been Tested?

  • Locally

Screenshots (if appropriate):

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

@IAmKio IAmKio requested a review from RanaBug July 3, 2024 13:38
@IAmKio IAmKio self-assigned this Jul 3, 2024
@cloudflare-workers-and-pages
Copy link

cloudflare-workers-and-pages bot commented Jul 3, 2024

Deploying x with  Cloudflare Pages  Cloudflare Pages

Latest commit: 44b399a
Status: ✅  Deploy successful!
Preview URL: https://e8abc3eb.x-e62.pages.dev
Branch Preview URL: https://improvement-navigation-core.x-e62.pages.dev

View logs


// hooks
import useAllowedApps from '../hooks/useAllowedApps';
// import useAllowedApps from '../hooks/useAllowedApps';
Copy link
Collaborator

Choose a reason for hiding this comment

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

remove comments


export const AuthorizedNavigation = () => {
const { allowed: allowedApps } = useAllowedApps();
// const { allowed: allowedApps } = useAllowedApps();
Copy link
Collaborator

Choose a reason for hiding this comment

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

remove


// Next, add the allowed apps to the route definition
allowedApps.forEach((appId) => {
authorizedRoutesDefinition[0].children.push({
Copy link
Collaborator

Choose a reason for hiding this comment

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

Not sure to understand here why we have twice authorizedRoutesDefinition[0].children.push() ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Here is the full block:

   allowedApps.forEach((appId) => {
      authorizedRoutesDefinition[0].children.push({
        path: `/${appId}`,
        element: <App id={appId} />,
      });
      authorizedRoutesDefinition[0].children.push({
        path: `/${appId}/*`, // <-- is this what you are referring to? there's a catch all route here
        element: <App id={appId} />,
      });
    });

Copy link
Collaborator

Choose a reason for hiding this comment

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

Aaaah I see, ok thank you

</Routes>
);
return <RouterProvider router={router} />
// return (
Copy link
Collaborator

Choose a reason for hiding this comment

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

delete commented code

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants