-
Notifications
You must be signed in to change notification settings - Fork 4k
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(dashboard): env change logic and routes #6632
Conversation
], | ||
}, | ||
]; | ||
export const buildNavigationItems = ({ |
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.
we need to dynamically build nav items because the workflows link depends on the current environment
if ( | ||
(pathname === ROUTES.ROOT || pathname === ROUTES.ENV || pathname === `${ROUTES.ENV}/`) && | ||
selectedEnvironment | ||
) { | ||
navigate(buildRoute(ROUTES.WORKFLOWS, { environmentId: selectedEnvironment._id })); | ||
return; | ||
} else if (pathname.includes(ROUTES.ENV) && selectedEnvironment) { | ||
const newPath = pathname.replace(/\/env\/[^/]+(\/|$)/, `${ROUTES.ENV}/${selectedEnvironment._id}$1`); | ||
navigate(newPath); | ||
return; | ||
} | ||
|
||
navigate(pathname, { replace: true }); |
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.
the navigation logic based on the current pathname handles the cases when the route is:
/
/env
/env/
/env/:environmentId
/env/:environmentId/
/env/:environmentId/workflows
/env/:environmentId/anything
For the cases when the environmentId
is present in the URL, it will replace it with the new environment id
useLayoutEffect(() => { | ||
if (!environments) { | ||
return; | ||
} | ||
|
||
const selectedEnvironment = selectEnvironment(environments, getEnvironmentId()); | ||
switchEnvironmentInternal(environments, selectedEnvironment?._id); | ||
}, [environments, switchEnvironmentInternal]); |
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.
when the environment is loaded, or when the component is mounted and we have environments in the query cache --> set the current environment and redirect the user
export const buildRoute = (route: string, params: Record<string, string>) => { | ||
return Object.entries(params).reduce((acc, [key, value]) => { | ||
return acc.replace(`:${key}`, value); | ||
}, route); |
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.
the helper to build the params based urls like: /env/:environmentId/workflows
✅ Deploy Preview for novu-stg-vite-dashboard-poc ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
return; | ||
} | ||
|
||
navigate(pathname, { replace: true }); |
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.
TODO: probably the last navigate
is not needed
const selectedEnvironment = selectEnvironment(allEnvironments, environmentId); | ||
setCurrentEnvironment(selectedEnvironment); |
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.
select the env based on the URL:
- if there is no env ID in the URL it will default to the development environment
- otherwise, it will select related env
if (pathname === ROUTES.ROOT || pathname === ROUTES.ENV || pathname === `${ROUTES.ENV}/`) { | ||
navigate(buildRoute(ROUTES.WORKFLOWS, { environmentId: newEnvironmentId })); |
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.
if URL is: /
or /env
or /env/
then redirect to the /env/:environmentId/workflows
export const buildNavigationItems = ({ | ||
currentEnvironment, | ||
}: { | ||
currentEnvironment?: IEnvironment | null; |
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.
Null shouldn't be allowed as a type. This method must be called only when the env is set. This will also simplify all the checks in the method.
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.
To ensure that, we can do exactly what we did in the current web
app, introduce a Guard component that doesn't allow rendering of the layout until Clerk + Env are resolved.
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.
@SokratisVidros, IMO, it creates a bad UX, especially since it will wait for two requests to complete. It would be better to render some loading indicator/skeleton or smth; I mentioned this here: https://novu.slack.com/archives/C07HVLG7G2U/p1728060743365279
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 agree 100% with the need for a splash/loading screen. That's exactly what the guard does in web
. We "server-side" render the loader via the public.html and then it is removed only when env is resolved. If not mistaken we are saying the same thing. See here
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.
If not mistaken we are saying the same thing
Not really, what I'm saying is to render the application shell while environments are loading: header, side nav, and in the main content, show the loading indicator. We can think about how it should look design-wise. I want to avoid empty screens, loading, blinking, etc., we can do better.
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 am aligned with that. The key is not to make the app interactive until Clerk + Env is loaded.
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.
The UX about the environment is different from allowing the null
type. I still suggest to remove the null
bit and prevent /env/undefined route generation.
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.
@SokratisVidros, we will solve it with UI state for the side nav while the env's are loading. Otherwise, I have to do the if
statement and return nothing, so there will be a blinking experience
@@ -35,18 +32,51 @@ function selectEnvironment(environments?: IEnvironment[], selectedEnvironmentId? | |||
|
|||
export function EnvironmentProvider({ children }: { children: React.ReactNode }) { | |||
const { currentOrganization } = useAuth(); | |||
const navigate = useNavigate(); | |||
const { pathname } = useLocation(); | |||
const { environmentId: paramsEnvironmentId } = useParams<{ environmentId?: string }>(); |
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.
Info: we will update the environmentId
later to slug-hash
when it will be available
export const buildNavigationItems = ({ | ||
currentEnvironment, | ||
}: { | ||
currentEnvironment?: IEnvironment | null; |
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.
The UX about the environment is different from allowing the null
type. I still suggest to remove the null
bit and prevent /env/undefined route generation.
What changed? Why was the change needed?
Switch environments logic and env-based URL routing.
Screenshots
Screen.Recording.2024-10-04.at.18.59.35.mov
Screen.Recording.2024-10-04.at.18.58.42.mov
Screen.Recording.2024-10-04.at.18.57.22.mov
Screen.Recording.2024-10-07.at.12.29.14.mov
Screen.Recording.2024-10-07.at.12.48.36.mov