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

fix(web): improved timed digest form behaviour #3405

Merged
merged 3 commits into from
May 14, 2023

Conversation

LetItRock
Copy link
Contributor

What change does this PR introduce?

Improved the timed digest form:

  • updated form schema
  • the digest/delay step metadata in the form state is split between digestMetadata and delayMetadata;
  • independent form state for the different digest tabs:
    • regular and timed
    • min, hour, daily, weekly, monthly;
    • thanks to this the values typed in the form are persisted when switching tabs
  • default value set on blur when the value is empty: regular digest - amount, backoff amount; timed digest - amount, at time
  • improved "Will be sent" field accordion header
  • code split into separate components
  • added missed default values

Why was this change needed?

This is a part of the Timed Digest PRD.

Other information (Screenshots)

Screenshot 2023-05-14 at 01 02 31

Screen.Recording.2023-05-14.at.00.59.00.mov
Screen.Recording.2023-05-14.at.01.00.46.mov
Screen.Recording.2023-05-14.at.01.01.50.mov

@LetItRock LetItRock self-assigned this May 13, 2023
@linear
Copy link

linear bot commented May 13, 2023

NV-2299 Fix the timed digest form

Reproduction Steps

Found a few problems with the form:

  • cannot change the value of scheduled to min
  • the every field state is not changing for different tabs: min, hour, daily, weekly, monthly
  • the form state is not preserved for each tabs: min, hour, daily, weekly, monthly

Expected Behaviour

@@ -13,6 +13,7 @@ export function useTemplateFetcher(
() => getTemplateById(templateId as string),
{
enabled: !!templateId,
refetchOnWindowFocus: false,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

do not refetch template on window focus

@@ -17,6 +17,7 @@ export const StepNameInput = ({ index, defaultValue }: { index: number; defaultV
<Controller
control={control}
name={`steps.${index}.name`}
defaultValue=""
Copy link
Contributor Author

Choose a reason for hiding this comment

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

missed default value, it's required by the react-hook-form... there are few more fields like this

Comment on lines +64 to 81
digestMetadata: {
digestKey: '',
type: DigestTypeEnum.REGULAR,
regular: {
unit: DigestUnitEnum.MINUTES,
amount: '5',
backoff: false,
},
},
}),
...(channelType === StepTypeEnum.DELAY && {
metadata: {
type: DigestTypeEnum.REGULAR,
delayMetadata: {
type: DelayTypeEnum.REGULAR,
regular: {
unit: DigestUnitEnum.MINUTES,
amount: '5',
},
},
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 default values for the digest/delay step metadata... I'll explain about these more later

Comment on lines +171 to +178
const { template, isLoading, isCreating, isUpdating, isDeleting, updateNotificationTemplate } = useTemplateController(
templateId,
{
onFetchSuccess: (fetchedTemplate) => {
setTrigger(fetchedTemplate.triggers[0]);
const form = mapNotificationTemplateToForm(fetchedTemplate);
reset(form);
},
Copy link
Contributor Author

Choose a reason for hiding this comment

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

only reset the form after we've fetched it... like update it with BE values

@@ -183,7 +194,7 @@ const TemplateEditorFormProvider = ({ children }) => {
},
});
setTrigger(response.triggers[0]);
reset(payloadToCreate);
reset(form);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

reset with the current form state, this removed dirty flags from the fields and "disables" the "update" button

Comment on lines +53 to +57
onBlur={(e) => {
if (e.target.value === '') {
setValue(amountFieldName, amountDefaultValue);
}
field.onBlur();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

set default values on blur

<span>Every</span>
<Controller
control={control}
key={amountFieldName}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

key is a trick to have a one "amount (every)" field and re-mount it when amountFieldName changes, and it changes when switching between the tabs: min, hours, days, weeks, months

</>
);
if (type === DigestTypeEnum.TIMED) {
return <TimedDigestWillBeSentHeader index={index} />;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

exported the timed digest logic to separate component...

}
setValue(`steps.${index}.metadata.backoff`, false);
}, [backoff, type, index]);
export const WillBeSentHeader = ({ index }: { index: 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.

"Will be sent" field accordion header, there are a lot of different logic to show different labels depending on what user has selected

@davidsoderberg davidsoderberg merged commit cf66860 into timed-digest May 14, 2023
32 of 33 checks passed
@davidsoderberg davidsoderberg deleted the nv-2299-fix-timed-digest-form branch May 14, 2023 04:31
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.

None yet

2 participants