-
Notifications
You must be signed in to change notification settings - Fork 7
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
Waitp 1202 setup routes #4608
Waitp 1202 setup routes #4608
Conversation
packages/tupaia-web/src/Router.tsx
Outdated
const Routes = [ | ||
{ | ||
path: '/', | ||
element: <Navigate to={`/${DEFAULT_URL}`} replace />, // This is to redirect from no URL to the default project URL. Some logic will also have to be added in here to render the projects modal in this case as well |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
great comments
packages/tupaia-web/src/Router.tsx
Outdated
}, | ||
// The below user pages will actually be modals, which will be done when each view is created. There is an example at: https://github.com/remix-run/react-router/tree/dev/examples/modal | ||
{ | ||
path: '/register', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I reckon we should put these specific hard coded urls above the Base url as they should always exist. We should also add this list of urls to a black list of codes that can't be used for project landing pages. Would you mind updating the constructNewRecordValidationRules.js with the extra validation rule.
const finalCharSubtractor = finalURLChar === '/' ? 1 : 0; | ||
|
||
// If a pathname has only 1 segment, treat as a landing page, otherwise treat as a project | ||
const isLandingPage = pathname.split('/').length - 1 - finalCharSubtractor === 1; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
const isLandingPage = pathname.split('/').length - 1 - finalCharSubtractor === 1; | |
const { entityCode } = useParams(); | |
const isLandingPage = !entityCode; |
What about just using the named param to check if it's a landing page?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually I think we can do away with Base altogether and just have this check in the Router
packages/tupaia-web/src/Router.tsx
Outdated
{ | ||
path: '/reset-password', | ||
element: <PasswordResetForm />, | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we also add one for verify-email
packages/tupaia-web/src/Router.tsx
Outdated
{ | ||
// This is the base view for anything with a code in the URL. This will render a landing page if the URL has only one segment, and if it has more than 1 segment it will render the MapView | ||
path: '/:code/:entityCode?/*', | ||
element: <Base />, | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
{ | |
// This is the base view for anything with a code in the URL. This will render a landing page if the URL has only one segment, and if it has more than 1 segment it will render the MapView | |
path: '/:code/:entityCode?/*', | |
element: <Base />, | |
}, | |
{ | |
// Render a landing page if the URL has only one segment | |
path: '/:code', | |
element: <LandingPage />, | |
}, | |
{ | |
// Render the main project page | |
path: '/:code/:entityCode/:dashboardCode?', | |
element: <MainPage /> (or maybe <ProjectPage />), | |
}, |
What do you think about setting the route like this instead of the base route to make it more transparent. And any reason not to include the dashboardCode param?
Also I'm open to suggestions on renaming the pages
folder to views or routes if you think that makes more sense?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work, it's so exciting to be using real routes! I left a couple of suggestions, let me know what you think. thanks
This reverts commit 607d341.
import { DEFAULT_URL } from './constants'; | ||
|
||
/** | ||
* This Router is using [version 6.3]{@link https://reactrouter.com/en/v6.3.0}, as later versions are not supported by our TS setup. See [this issue here]{@link https://github.com/remix-run/react-router/discussions/8364} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks for the comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good - great work!
Issue WAITP-1202: Setup routing for main routes
Changes:
tupaia-web
, for user pages, project, and landing page, including handling the cases of similar routes (projects and landing pages)Note: This is just basic logic, there will need to be more logic added in around modals and so on when these are implemented.