-
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): workflow editor route and basic layout #6681
Conversation
'@typescript-eslint/naming-convention': [ | ||
'error', | ||
{ | ||
filter: '_', | ||
selector: 'variableLike', | ||
leadingUnderscore: 'allow', | ||
format: ['PascalCase', 'camelCase', 'UPPER_CASE'], |
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.
allow using _
for the unused variables
return ( | ||
<IntercomProvider appId={INTERCOM_APP_ID}> | ||
<div className="relative flex w-full"> | ||
<div className="flex flex-1 flex-col overflow-y-auto overflow-x-hidden"> | ||
<HeaderNavigation startItems={headerStartItems} /> | ||
|
||
<div className="flex min-h-dvh flex-col overflow-y-auto overflow-x-hidden">{children}</div> | ||
</div> | ||
</div> | ||
</IntercomProvider> |
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.
similar layout to the dashboard layout, but we might change here the header navigation and we don't need to show the side navigation
@@ -0,0 +1,50 @@ | |||
import * as React from 'react'; |
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 tabs component updated according to our designs
const { workflowId } = useParams<{ workflowId?: string }>(); | ||
const navigate = useNavigate(); | ||
const form = useForm<z.infer<typeof formSchema>>({ mode: 'onSubmit', resolver: zodResolver(formSchema) }); | ||
const { handleSubmit, reset } = form; | ||
|
||
const { workflow } = useFetchWorkflow({ | ||
workflowId, | ||
onSuccess: (data) => { | ||
reset(data); | ||
}, | ||
onError: () => { | ||
navigate(buildRoute(ROUTES.WORKFLOWS, { environmentId: currentEnvironment?._id ?? '' })); | ||
}, | ||
}); |
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.
fetch the workflow and set it in the form state
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 BE returns an error, navigate the user to the workflow list page
<TruncatedText | ||
className="cursor-pointer" | ||
text={workflow.name} | ||
onClick={() => { | ||
navigate( | ||
buildRoute(ROUTES.EDIT_WORKFLOW, { | ||
environmentId: currentEnvironment?._id ?? '', | ||
workflowId: workflow._id, | ||
}) | ||
); | ||
}} | ||
/> |
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 wasn't sure if this is what it should be because, from the designs, it's not clear. But decided to do it like when clicking on the workflow name, redirect the user to the workflow editor.
novu
@novu/client
@novu/headless
@novu/framework
@novu/js
@novu/nest
@novu/nextjs
@novu/notification-center
@novu/node
@novu/react
@novu/providers
@novu/react-native
@novu/shared
@novu/stateless
commit: |
✅ Deploy Preview for novu-stg-vite-dashboard-poc ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
apps/dashboard/src/api/bridge.ts
Outdated
import { get, post } from './api.client'; | ||
|
||
export const getBridgeHealthCheck = async () => { | ||
const { data } = await get<{ data: BridgeStatus }>('/bridge/status'); |
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 return errors as well. Then in a separate PR we need to wire them to a global error handler.
@@ -0,0 +1,124 @@ | |||
import { useLayoutEffect, useState } from 'react'; | |||
import { RiLinkM, RiPencilFill } from 'react-icons/ri'; | |||
import { PopoverPortal } from '@radix-ui/react-popover'; |
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.
Is this the recommended way to use a PopoverPortal for ShadCN? I'd expect this to come from primitives folder leveraging the ShadCN code downloaded via the CLI.
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.
Unfortunately, the cli doesn't generate the portal 🤷♂️ but we can re-export it, I'll do it
await updateBridgeUrl({ url: bridgeUrl, environmentId: currentEnvironment?._id ?? '' }); | ||
setBridgeUrl(bridgeUrl); | ||
} else { | ||
setError('bridgeUrl', { message: 'The provided URL is not the Novu Endpoint URL' }); |
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 new API, the team, prepared has discrete error types. So we should use the server response instead of hardcoding a frontend error message.
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.
@SokratisVidros, the updateBridgeUrl
is calling the /environments/:environmentId
endpoint. Do I miss something?
apps/dashboard/src/components/header-navigation/edit-bridge-url-button.tsx
Outdated
Show resolved
Hide resolved
apps/dashboard/src/components/header-navigation/edit-bridge-url-button.tsx
Outdated
Show resolved
Hide resolved
onClick={() => { | ||
navigate( | ||
buildRoute(ROUTES.EDIT_WORKFLOW, { | ||
environmentId: currentEnvironment?._id ?? '', |
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 pure inner DX but also a way to prevent invalid states in the front end, such as a workflow route, without environmentId.
Can we please update the hook times so that they always return a currentEnvironment when used to avoid this ternary on each current environment?
There are two ways to go about this.
-
The hook with throw an error if used and the environment is not set (fail fast), but the environment return type will always be
IEnvironment
. -
We can teach the hook to return the
{isLoaded, currentEnvironment}
tuple and use discriminated unions so that ifisLoaded
is true currentEnvironment is always an IEnvironment. Then, in all the components that useuseEnvironment
we can doif (!isLoaded) { return 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.
@SokratisVidros, it's not that simple:
- Throwing an error won't work, because on the initial render the
currentEnvironment
is always undefined, so theuseEnvironment
hook will always throw. - That's a partial solution to this problem because all code above the
if (!isLoaded)
including hooks will still need to check whether the current env is present.
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 will take this offline. It's not a good enough blocker for this PR.
What changed? Why was the change needed?
Workflow Editor route and basic layout
Screenshots
Screen.Recording.2024-10-11.at.17.01.46.mov