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

Nv 2996 reusable push preview component #5178

Merged

Conversation

BiswaViraj
Copy link
Member

@BiswaViraj BiswaViraj commented Feb 12, 2024

What change does this PR introduce?

  • Reusable Push preview component
  • reusable hook to get combined errors
  • fixes to the overlay blur designs

Why was this change needed?

Other information (Screenshots)

image

image

image

image

image

image

image

image

Screen.Recording.2024-02-12.at.12.47.21.PM.mov

Copy link

linear bot commented Feb 12, 2024

@@ -119,6 +119,7 @@ export class ContentTemplatesController {
public previewPush(
@UserSession() user: IJwtPayload,
@Body('content') content: string,
@Body('title') title: string,
Copy link
Member Author

Choose a reason for hiding this comment

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

Added title parsing support for push previews

</Stack>
</When>
</Group>
<ContentAndOVerlayWrapperStyled onMouseEnter={handleMouseEnter} onMouseLeave={handleMouseLeave}>
Copy link
Member Author

Choose a reason for hiding this comment

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

Added this to have better blur effect on hover

@@ -75,14 +76,14 @@ export function ChatPreview() {

return (
<div>
<div>
<Flex>
Copy link
Member Author

Choose a reason for hiding this comment

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

Forces the LocaleSelect to not take the Full-width of the available space

Comment on lines 80 to 86
* combining title and content to get locales based upon variables in both title and content
* The api is not concerned about the content type, it will parse the given string and return the locales
*/
const combinedContent = `${previewData.current.title} ${previewData.current.content}`;
getLocalesFromContent({
content: combinedContent,
});
Copy link
Member Author

Choose a reason for hiding this comment

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

The API parses any given string and returns the locales of the translation groups variable used in the given string. So it is okay to combine the title and content string into one to pass it to the API.

Comment on lines 37 to 38

const templateContentError = useStepFormCombinedErrors();
Copy link
Member Author

Choose a reason for hiding this comment

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

updated it to use the combined errors hooks

Copy link
Member Author

Choose a reason for hiding this comment

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

Added this hook to get combined errors.
For example, when "content is missing" and "title is missing"
Instead of showing only one of them or both in different lines.
This hook will combine the error to Content and Title are missing. if single error, then it just returns the single error like Content is missing.

We already use this behaviour in our Workflow nodes error. this just adds a reusable hook to get those formatted errors

Comment on lines +74 to +83
export function mapStepErrors(stepErrors) {
if (stepErrors) {
const keys = Object.keys(stepErrors);

return keys.map((key) => stepErrors[key]?.message);
}

return [];
}

Copy link
Member Author

Choose a reason for hiding this comment

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

moved it into a separate function to use it in the hook

Comment on lines +51 to +54

if (command.title) {
title = await this.compileStepTemplate(command.title, payload);
}
Copy link
Member Author

Choose a reason for hiding this comment

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

Handles push title parsing

};

return (
<ContentAndOVerlayWrapperStyled onMouseEnter={handleMouseEnter} onMouseLeave={handleMouseLeave}>
Copy link
Member Author

Choose a reason for hiding this comment

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

Added this to have better blur effect on hover

@@ -75,14 +76,14 @@ export function ChatPreview() {

return (
<div>
<div>
<Flex>
Copy link
Member Author

Choose a reason for hiding this comment

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

Forces the LocaleSelect to not take the Full-width of the available space

Comment on lines 79 to 87
/*
* combining title and content to get locales based upon variables in both title and content
* The api is not concerned about the content type, it will parse the given string and return the locales
*/
const combinedContent = `${previewData.current.title} ${previewData.current.content}`;
getLocalesFromContent({
content: combinedContent,
});
}
Copy link
Member Author

Choose a reason for hiding this comment

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

The API parses any given string and returns the locales of the translation groups variable used in the given string. So it is okay to combine the title and content string into one to pass it to the API.

@@ -33,7 +34,8 @@ const LocaleSelectStyled = styled(LocaleSelect)`
export const SmsPreview = () => {
const { control } = useFormContext<IForm>();
const path = useStepFormPath();
const error = useStepFormErrors();

const templateContentError = useStepFormCombinedErrors();
Copy link
Member Author

Choose a reason for hiding this comment

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

updated it to use the combined errors hooks

Copy link
Member Author

Choose a reason for hiding this comment

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

Added this hook to get combined errors.
For example, when "content is missing" and "title is missing"
Instead of showing only one of them or both in different lines.
This hook will combine the error to Content and Title are missing. if single error, then it just returns the single error like Content is missing.

We already use this behaviour in our Workflow nodes error. this just adds a reusable hook to get those formatted errors

@BiswaViraj BiswaViraj requested review from LetItRock and ainouzgali and removed request for LetItRock February 12, 2024 08:14
Copy link
Contributor

@LetItRock LetItRock left a comment

Choose a reason for hiding this comment

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

hey Biswa, can we please run the images through: https://squoosh.app/ and generate webp instead? we will save a lot in size
https://caniuse.com/?search=webp

const path = useStepFormPath();
const errorMsg = useStepFormCombinedErrors();

const [selectedLocale, setSelectedLocale] = useState<string | undefined>(undefined);
Copy link
Contributor

Choose a reason for hiding this comment

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

please take a look at this hook useTemplateLocales

apps/web/src/components/workflow/preview/push/Content.tsx Outdated Show resolved Hide resolved
@BiswaViraj BiswaViraj requested a review from LetItRock February 13, 2024 08:48
Copy link
Contributor

@LetItRock LetItRock left a comment

Choose a reason for hiding this comment

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

looks good to me! 🔥

Base automatically changed from nv-2998-reusable-sms-preview to translation-management-beta February 13, 2024 12:48
@LetItRock LetItRock merged commit 6fecdd7 into translation-management-beta Feb 13, 2024
23 checks passed
@LetItRock LetItRock deleted the nv-2996-reusable-push-preview-component branch February 13, 2024 14:23
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