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.

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): scheduled digest #7314

Open
wants to merge 6 commits into
base: next
Choose a base branch
from
Open

Conversation

LetItRock
Copy link
Contributor

@LetItRock LetItRock commented Dec 17, 2024

What changed? Why was the change needed?

Scheduled digest:

Screenshots

Screen.Recording.2024-12-17.at.12.43.57.mov
Screen.Recording.2024-12-17.at.12.45.39.mov

Copy link

linear bot commented Dec 17, 2024

Copy link

netlify bot commented Dec 17, 2024

Deploy Preview for dev-web-novu ready!

Name Link
🔨 Latest commit 3b2b19f
🔍 Latest deploy log https://app.netlify.com/sites/dev-web-novu/deploys/6762c871613695000823e7eb
😎 Deploy Preview https://deploy-preview-7314.dashboard.novu-staging.co
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link

netlify bot commented Dec 17, 2024

Deploy Preview for dashboard-v2-novu-staging ready!

Name Link
🔨 Latest commit 3b2b19f
🔍 Latest deploy log https://app.netlify.com/sites/dashboard-v2-novu-staging/deploys/6762c871f97f4e0008e07e24
😎 Deploy Preview https://deploy-preview-7314.dashboard-v2.novu-staging.co
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@@ -16,7 +16,7 @@
rel="stylesheet"
/>
<link
href="https://fonts.googleapis.com/css2?family=Ubuntu+Mono:ital,wght@0,400;0,700;1,400;1,700&display=swap"
href="https://fonts.googleapis.com/css2?family=JetBrains+Mono:ital,wght@0,400;0,700;1,400;1,700&display=swap"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

the font used for the amount input

@@ -69,11 +70,13 @@
"class-variance-authority": "^0.7.0",
"clsx": "^2.1.1",
"cmdk": "1.0.0",
"cron-parser": "^4.9.0",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

the library used to parse and generate cron expression

shouldUnregister?: boolean;
};

const AmountInputContainer = ({
Copy link
Contributor Author

Choose a reason for hiding this comment

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

All the changes I made here are: split components into smaller independent pieces so they can be reused without being dependent on the form

@@ -61,7 +61,7 @@ const FormLabel = React.forwardRef<
>
<BsFillInfoCircleFill className="text-foreground-300 -mt-0.5 inline size-3" />
</TooltipTrigger>
<TooltipContent>{tooltip}</TooltipContent>
<TooltipContent className="max-w-56">{tooltip}</TooltipContent>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Adjust the max width of the tooltip content so it doesn't look weird

import { Command, CommandGroup, CommandItem, CommandList } from '@/components/primitives/command';
import { selectTriggerVariants } from '@/components/primitives/select';

export const MultiSelect = <T extends string | number>({
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The dropdown component where the user can pick multiple values.

Comment on lines +54 to +70
const { second, month, dayOfMonth, dayOfWeek, hour, minute } = useMemo(() => {
try {
const expression = cronParser.parseExpression(value);
return toUiFields(expression.fields);
} catch (e) {
onError?.(e);

return {
second: [],
minute: [],
hour: [],
dayOfMonth: [],
month: [],
dayOfWeek: [],
};
}
}, [value, onError]);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Parse the cron value and get the form field values.

Comment on lines +72 to +84
const handleValueChange = (fields: Partial<UiCronFields>) => {
const cronFields = toCronFields({
second,
minute,
hour,
dayOfWeek,
dayOfMonth,
month,
...fields,
});

onValueChange(cronParser.fieldsToExpression(cronFields).stringify());
};
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We will generate a new cron expression when any value is changed on the UI.

};

const handlePeriodChange = (period: string) => {
onValueChange(getCronBasedOnPeriod(period as PeriodValues, { second, minute, hour, dayOfWeek, dayOfMonth, month }));
Copy link
Contributor Author

Choose a reason for hiding this comment

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

When the period value changes, we need to generate a new cron expression but preserve the previously selected user values.

/**
* regular expression to check for valid hour format (01-23)
*/
export function isValidHour(value: string) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This file utils are used by the TimePicker component

@@ -0,0 +1,403 @@
import cronParser, {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

These are the utils used by the scheduled digest UI to calculate the period from the cron and based on the current period value selected (when switching).
There are also utils that convert the cron fields to UI field values because the cron-parser for * generates the array with all values, but on the UI, we use an empty array as a sign for * basically every day, month, hour etc.

@@ -10,7 +10,7 @@ export const EditWorkflowPage = () => {
<EditWorkflowLayout headerStartItems={<EditorBreadcrumbs />}>
<div className="flex h-full flex-1 flex-nowrap">
<WorkflowTabs />
<aside className="text-foreground-950 [&_textarea]:text-neutral-600'; flex h-full w-[300px] max-w-[350px] flex-col border-l [&_input]:text-xs [&_input]:text-neutral-600 [&_label]:text-xs [&_label]:font-medium [&_textarea]:text-xs">
<aside className="text-foreground-950 [&_textarea]:text-neutral-600'; flex h-full w-[350px] max-w-[350px] flex-col border-l [&_input]:text-xs [&_input]:text-neutral-600 [&_label]:text-xs [&_label]:font-medium [&_textarea]:text-xs">
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unfortunately, I had to make the sidebar wider, otherwise the scheduled digest UI doesn't fit and look well.

Copy link

pkg-pr-new bot commented Dec 17, 2024

Open in Stackblitz

@novu/client

npm i https://pkg.pr.new/novuhq/novu/@novu/client@7314

@novu/headless

npm i https://pkg.pr.new/novuhq/novu/@novu/headless@7314

@novu/framework

npm i https://pkg.pr.new/novuhq/novu/@novu/framework@7314

@novu/js

npm i https://pkg.pr.new/novuhq/novu/@novu/js@7314

@novu/nest

npm i https://pkg.pr.new/novuhq/novu/@novu/nest@7314

@novu/nextjs

npm i https://pkg.pr.new/novuhq/novu/@novu/nextjs@7314

@novu/node

npm i https://pkg.pr.new/novuhq/novu/@novu/node@7314

@novu/notification-center

npm i https://pkg.pr.new/novuhq/novu/@novu/notification-center@7314

novu

npm i https://pkg.pr.new/novuhq/novu@7314

@novu/providers

npm i https://pkg.pr.new/novuhq/novu/@novu/providers@7314

@novu/react

npm i https://pkg.pr.new/novuhq/novu/@novu/react@7314

@novu/react-native

npm i https://pkg.pr.new/novuhq/novu/@novu/react-native@7314

@novu/shared

npm i https://pkg.pr.new/novuhq/novu/@novu/shared@7314

@novu/stateless

npm i https://pkg.pr.new/novuhq/novu/@novu/stateless@7314

commit: 3b2b19f

Copy link
Member

@BiswaViraj BiswaViraj left a comment

Choose a reason for hiding this comment

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

Amazing work on this 🔥

Left a couple of comments.

Screen.Recording.2024-12-18.at.6.24.43.PM.mov

The date selector is a little buggy for the "February" month. I also think we should hide the 30/31 date for feb and "31" for appropriate months. or just have a disabled UI state. as currently it allows selecting the date but the apply button remains stucked.

Comment on lines +51 to +62
setDigestType(value);
if (value === SCHEDULED_DIGEST_TYPE) {
setValue(AMOUNT_KEY, undefined, { shouldDirty: true });
setValue(UNIT_KEY, undefined, { shouldDirty: true });
setValue(CRON_KEY, EVERY_MINUTE_CRON, { shouldDirty: true });
} else {
setValue(AMOUNT_KEY, '', { shouldDirty: true });
setValue(UNIT_KEY, TimeUnitEnum.SECONDS, { shouldDirty: true });
setValue(CRON_KEY, undefined, { shouldDirty: true });
}
await trigger();
saveForm();
Copy link
Member

Choose a reason for hiding this comment

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

I suggest adding a confirmation popup window when switching the digest tabs.
especially when switching from "schedule" to "regular"
As someone might mis-click it and lose all the progress saved in the scheduled form.

Screen.Recording.2024-12-18.at.6.21.01.PM.mov

@LetItRock
Copy link
Contributor Author

The date selector is a little buggy for the "February" month. I also think we should hide the 30/31 date for feb and "31" for appropriate months. or just have a disabled UI state. as currently it allows selecting the date but the apply button remains stucked.

@BiswaViraj
The "year" period allows you to pick multiple months when your digest (cron) will run; that's why it's not aligned with the particular days in the month. It's implemented according to the functionality of cron expressions which allow selecting 31 day of February.

I suggest adding a confirmation popup window when switching the digest tabs.
especially when switching from "schedule" to "regular"
As someone might mis-click it and lose all the progress saved in the scheduled form.

I agree. I'll talk to Naveen and Radek about that, and we will do it in a separate PR.

@LetItRock LetItRock requested a review from BiswaViraj December 18, 2024 13:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants