fix: Added Warning and Error icons to Worlflow settings component #4413

rayy40 · 2023-10-04T22:21:13Z

What change does this PR introduce?

This PR improves the UX of the workflow settings component, by using Warning Icon for warning and Error Icon for error.

Why was this change needed?

The change was to improve the UX of the workflow settings component and add the icons according to the designs.

Screenshots

I wasn't able to get the screenshot for the yellow warning banner for the SMS or Email channels when there is only one Novu SMS/Email integration since the Novu Email/SMS provider was disabled because the free Novu provider allows sending 0 emails per month.

davidsoderberg · 2023-10-05T02:53:14Z

Hi @rayy40 I think it was included in the issue to update the logic to show the yellow banner even if novu provider is there. The definition of done says "show the yellow warning banner only for the SMS or Email channels if there is only one Novu SMS/Email integration". Please tell me if you need help to find that logic :)

rayy40 · 2023-10-05T07:38:09Z

Hey @davidsoderberg, as I checked I feel like the yellow warning banner will be shown for the SMS or Email channels, I just wasn't able to reproduce it to take a screenshot of the same. I wasn't able to reproduce it because the Novu Email provider was disabled since the free Novu provider doesn't allow sending of emails.

In the ListProviders component the alertType is set to warning when this is true

const containsNovuProvider = NOVU_SMS_EMAIL_PROVIDERS.some(
    (providerId) => providerId === providers.find((provider) => provider.connected)?.providerId
  );

providers.filter((provider) => provider.connected).length === 1 && containsNovuProvider

So, it already checks if it contains a Novu Provider and sets the type as warning, and then passes it to LackIntegrationAlert component where the background color is being changed based on the type.

<WarningMessage backgroundColor={alertTypeToMessageBackgroundColor(type)}>
 
function alertTypeToMessageBackgroundColor(type: alertType) {
  switch (type) {
    case 'error':
      return 'rgba(229, 69, 69, 0.15)';
    case 'warning':
      return 'rgba(234, 169, 0, 0.15)';
    default:
      return 'rgba(229, 69, 69, 0.15)';
  }
}

If I am missing something or mistaking something, please let me know I'd be happy to take a look into that.

davidsoderberg · 2023-10-05T07:43:37Z

Hey @davidsoderberg, as I checked I feel like the yellow warning banner will be showed for the SMS or Email channels, I just wasn't able to reproduce it to take a screenshot of the same. I wasn't able to reproduce it because the Novu Email provider was disabled since the free Novu provider doesn't allow sending of emails.

In the ListProviders component the alertType is set to warning when this is true
const containsNovuProvider = NOVU_SMS_EMAIL_PROVIDERS.some(
    (providerId) => providerId === providers.find((provider) => provider.connected)?.providerId
  );

providers.filter((provider) => provider.connected).length === 1 && containsNovuProvider
 
So, it already checks if it contains a Novu Provider and sets the type as warning, and then passes it to LackIntegrationAlert component where the background color is being changed based on the type.
<WarningMessage backgroundColor={alertTypeToMessageBackgroundColor(type)}>
 
function alertTypeToMessageBackgroundColor(type: alertType) {
  switch (type) {
    case 'error':
      return 'rgba(229, 69, 69, 0.15)';
    case 'warning':
      return 'rgba(234, 169, 0, 0.15)';
    default:
      return 'rgba(229, 69, 69, 0.15)';
  }
}
If I am missing something or mistaking something, please let me know I'd be happy to take a look into that.

A yes that makes sense. Will test it out later and see if I can get them to show.

davidsoderberg · 2023-10-05T10:59:07Z

Okay I managed to get it to show up, Great work this far 🥳

Text color is not set for warning and the color for warning icon seems to be orange now. Please fix those and it feels ready to be merged 😄

rayy40 · 2023-10-05T23:12:12Z

Hey @davidsoderberg I set the text color of warning and also fixed the color of Warning Icon.

I just was curious about one thing, In the design-system/config/colors file the color for warning is set as warning: '#FF8000'
But according to the the design, shouldn't it be set as warning: #EAA900
I did not change that because I wasn't sure if the warning is used anywhere else and needs that color.

davidsoderberg · 2023-10-06T02:46:14Z

Hey @davidsoderberg I set the text color of warning and also fixed the color of Warning Icon.

I just was curious about one thing, In the design-system/config/colors file the color for warning is set as warning: '#FF8000' But according to the the design, shouldn't it be set as warning: #EAA900 I did not change that because I wasn't sure if the warning is used anywhere else and needs that color.

I think we can make color a props and make the orange color default value.

…/rayy40/novu into fix/add-icons-to-alert-component

davidsoderberg · 2023-10-06T06:47:18Z

apps/web/src/design-system/config/colors.ts

@@ -18,7 +18,7 @@ export const colors = {
  gradientStart,
  gradientEnd,
  success: '#4D9980',
-  warning: '#FF8000',
+  warning: '#EAA900',


Not sure we can replace it here can you pass it a prop instead and make the "FF8000" as default color prop?

I have already passed it as a prop in my previous commit.

<WarningIcon color={color} onClick={onClick} />; export function WarningIcon(props: React.ComponentPropsWithoutRef<'svg'>) { return ( <svg width="18" height="15" viewBox="0 0 18 15" fill="none" xmlns="http://www.w3.org/2000/svg" {...props}> <path d="M16.8125 13.0312L10.125 1.65625C9.625 0.78125 8.34375 0.78125 7.8125 1.65625L1.15625 13.0312C0.65625 13.9062 1.28125 15 2.3125 15H15.6562C16.6875 15 17.3125 13.9062 16.8125 13.0312ZM8.25 5.25C8.25 4.84375 8.5625 4.5 9 4.5C9.40625 4.5 9.75 4.84375 9.75 5.25V9.25C9.75 9.6875 9.40625 10 9 10C8.5625 10 8.25 9.6875 8.25 9.25V5.25ZM9 13C8.4375 13 8 12.5625 8 12.0312C8 11.5 8.4375 11.0625 9 11.0625C9.53125 11.0625 9.96875 11.5 9.96875 12.0312C9.96875 12.5625 9.53125 13 9 13Z" fill="currentColor" /> </svg> );

Is this what you mean?

Ah yes but lets not change the color in colors but hardcoded it for now where you need it.

Made the changes as required.

rayy40 · 2023-10-06T06:48:02Z

I think we can make color a props and make the orange color default value.

I have made the required changes.

…/rayy40/novu into fix/add-icons-to-alert-component

rayy40 · 2023-10-08T20:50:56Z

Hey @davidsoderberg , any updates?

davidsoderberg · 2023-10-09T05:16:06Z

There is an issue where WarningIcon is used in other places it does not get a color at all, please fix this :)

rayy40 · 2023-10-09T06:44:44Z

There is an issue where WarningIcon is used in other places it does not get a color at all, please fix this :)

Okay, I get it.
I need to set a default color to the WarningIcon in case no color prop is being passed.
Can you please tell me which default color should I use?
Should it be the warning color from colors.ts file, or should it be the color from the designs?

These are the two hex codes:

'#FF8000',
'#EAA900',

davidsoderberg · 2023-10-09T06:50:49Z

There is an issue where WarningIcon is used in other places it does not get a color at all, please fix this :)

Okay, I get it. I need to set a default color to the WarningIcon in case no color prop is being passed. Can you please tell me which default color should I use? Should it be the warning color from colors.ts file, or should it be the color from the designs?

These are the two hex codes:
'#FF8000',
'#EAA900',

It needs to be the one in colors.ts and then in your case you need to pass the new one in figma as color prop.

rayy40 · 2023-10-09T07:45:33Z

It needs to be the one in colors.ts and then in your case you need to pass the new one in figma as color prop.

I have fixed the error, can you please review it now? :)

rayy40 · 2023-10-11T07:49:59Z

Hey @davidsoderberg , Are there any other changes required?

LetItRock · 2023-10-11T19:41:09Z

hey @rayy40 👋 thank you so much for the incredible work you did 🤝

Could you please do me a favor and add a few changes that I've missed from the current implementation? 🙏

remove the additional div around the AlertIcon - it breaks the centering
place the width="22px" height="22px" props on the warning and error icons (the warning rn is smaller)
in the WarningMessage add the cursor: pointer; rule
place the onClick prop from the AlertIcon to the WarningMessage
add the missing "circle with arrow" icon (for both types)

Thanks in advance and sorry about that 🙏

rayy40 · 2023-10-13T23:15:34Z

Hey @LetItRock , can you please review the changes?

ainouzgali

Hi @rayy40 :)
A few notes:

Please take a look at how the alert shows in the editor sidebar itself (for example sms editor), the icons are very small there.

The warning banner (the yellow) is shown before the novu provider in the providers list. It should show first the novu provider and under it the warning.

ainouzgali · 2023-10-15T11:56:44Z

apps/web/src/pages/templates/components/LackIntegrationAlert.tsx

          <Text color={alertTypeToMessageTextColor(type)}>
            {text
              ? text
              : `Please configure or activate a provider instance for the ${stepNames[channelType]} channel to send notifications over this node`}
          </Text>
+          <CircleArrowRight />


The correct color should be passed here.

Also, I believe this arrow will be shown immediately after the text, and will not at the end of the row like it should.

Also, I believe this arrow will be shown immediately after the text, and will not at the end of the row like it should.

When placing it just after the text, I need to add another flex container to align the items, and I feel like it's redundant since there is already a flex container Group which is a parent for AlertIcons and Text, so I just added CircleArrowRight there too.

Please let me know if I am missing something or need to go with a different approach.

@rayy40, in the comment below I added a screenshot 0f your result, as you can see the arrow is not at the end of the row :)

@ainouzgali , Ah sorry. I wasn't able to understand you before ;(

ainouzgali · 2023-10-15T12:23:19Z

Hey @davidsoderberg, as I checked I feel like the yellow warning banner will be shown for the SMS or Email channels, I just wasn't able to reproduce it to take a screenshot of the same. I wasn't able to reproduce it because the Novu Email provider was disabled since the free Novu provider doesn't allow sending of emails.

In the ListProviders component the alertType is set to warning when this is true
const containsNovuProvider = NOVU_SMS_EMAIL_PROVIDERS.some(
    (providerId) => providerId === providers.find((provider) => provider.connected)?.providerId
  );

providers.filter((provider) => provider.connected).length === 1 && containsNovuProvider
 
So, it already checks if it contains a Novu Provider and sets the type as warning, and then passes it to LackIntegrationAlert component where the background color is being changed based on the type.

If I am missing something or mistaking something, please let me know I'd be happy to take a look into that.

In addition to that logic, you should add a check for the environment. Because currently it will find both the production and development novu integration so the length will not be 1 and the warning banner will not be shown

rayy40 · 2023-10-16T07:34:36Z

The warning banner (the yellow) is shown before the novu provider in the providers list. It should show first the novu provider and under it the warning.

Hey @ainouzgali , could you please tell me how to reproduce this issue?

ainouzgali · 2023-10-16T08:30:05Z

The warning banner (the yellow) is shown before the novu provider in the providers list. It should show first the novu provider and under it the warning.

Hey @ainouzgali , could you please tell me how to reproduce this issue?

@rayy40 , after you add the environment check I told you about in ListProviders.tsx. Should be something like this:

<When
        truthy={
          providers.filter((provider) => provider.connected && provider.environmentId === currentEnvironment?._id)
            .length === 1 && containsNovuProvider
        }
      >
        <div
          style={{
            marginBottom: -28,
          }}
        >
          <LackIntegrationAlert text={'Connect a provider for this channel'} channelType={channel} type={'warning'} />
        </div>
      </When>

That will make the warning alert show at all.
Now, you will see that it results in the warning alert shown on the list before the novu provider

According to the design, it should be shown below

rayy40 · 2023-10-16T20:05:45Z

@ainouzgali , I have actually added the check for environment but what I meant was, how can I check for the warning message? Since currently I am unable to activate the novu email provider in the development environment? As you have shown in your screenshot, that you have connected the novu email provider and getting the warning message.

…e top

rayy40 · 2023-10-16T21:14:42Z

@ainouzgali , I have actually added the check for environment but what I meant was, how can I check for the warning message? Since currently I am unable to activate the novu email provider in the development environment? As you have shown in your screenshot, that you have connected the novu email provider and getting the warning message.

I guess, I figured why the warning alert was being shown at the top. Please take a look at it, I was not able to check it myself since couldn't figure out if it was possible to enable the novu email provider.

ainouzgali

It looks great!! Awesome job!
Just remove the duplicate function and we will merge this⭐️

apps/web/src/pages/templates/components/LackIntegrationAlert.tsx

rayy40 · 2023-10-17T19:15:19Z

Hey @ainouzgali , I have removed the duplicate function. Can you please take a look? :)

ainouzgali

Thank you @rayy40 ! we appreciate it!
Great work

fix: Added Warning and Error icons to Worlflow settings component

c32f6c9

github-actions bot added @novu/web community labels Oct 4, 2023

Merge branch 'next' into fix/add-icons-to-alert-component

ed9ee69

Merge branch 'next' into fix/add-icons-to-alert-component

83b9a7c

fix: Added the text color of warning and fixed the color of Warning Icon

e5607f6

Merge branch 'next' into fix/add-icons-to-alert-component

cb66538

rayy40 added 3 commits October 6, 2023 12:13

fix: Changed the color of warning color prop

40e1fa4

Merge branch 'fix/add-icons-to-alert-component' of https://github.com…

dfd7feb

…/rayy40/novu into fix/add-icons-to-alert-component

Merge branch 'next' into fix/add-icons-to-alert-component

b4343d3

davidsoderberg reviewed Oct 6, 2023

View reviewed changes

rayy40 added 2 commits October 6, 2023 12:24

fix: Reverted back to the default color for warning color

19d4462

Merge branch 'fix/add-icons-to-alert-component' of https://github.com…

fb6b9f9

…/rayy40/novu into fix/add-icons-to-alert-component

fix: WarningIcon not showing color in other areas

42a433b

Merge branch 'next' into fix/add-icons-to-alert-component

b352cc6

Merge branch 'next' into fix/add-icons-to-alert-component

19e4bff

fix: Updated some changes for the ux

daa3d9a

Merge branch 'next' into fix/add-icons-to-alert-component

d345560

ainouzgali reviewed Oct 15, 2023

View reviewed changes

fix: Arrow icon at the end of row and Warning alert being shown at th…

7c7d818

…e top

Merge branch 'next' into fix/add-icons-to-alert-component

ca27b0c

ainouzgali reviewed Oct 17, 2023

View reviewed changes

apps/web/src/pages/templates/components/LackIntegrationAlert.tsx Outdated Show resolved Hide resolved

rayy40 added 2 commits October 18, 2023 00:42

fix: Remove duplicate function

a12f61a

Merge branch 'next' into fix/add-icons-to-alert-component

abfa50a

rayy40 requested a review from ainouzgali October 20, 2023 07:55

Merge branch 'next' into fix/add-icons-to-alert-component

1d7aee8

ainouzgali approved these changes Oct 22, 2023

View reviewed changes

ainouzgali merged commit 11e9b3f into novuhq:next Oct 22, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: Added Warning and Error icons to Worlflow settings component #4413

fix: Added Warning and Error icons to Worlflow settings component #4413

rayy40 commented Oct 4, 2023 •

edited

Loading

davidsoderberg commented Oct 5, 2023

rayy40 commented Oct 5, 2023 •

edited

Loading

davidsoderberg commented Oct 5, 2023

davidsoderberg commented Oct 5, 2023

rayy40 commented Oct 5, 2023

davidsoderberg commented Oct 6, 2023

davidsoderberg Oct 6, 2023

rayy40 Oct 6, 2023

davidsoderberg Oct 6, 2023 •

edited

Loading

rayy40 Oct 6, 2023

rayy40 commented Oct 6, 2023

rayy40 commented Oct 8, 2023

davidsoderberg commented Oct 9, 2023 •

edited

Loading

rayy40 commented Oct 9, 2023

davidsoderberg commented Oct 9, 2023

rayy40 commented Oct 9, 2023

rayy40 commented Oct 11, 2023

LetItRock commented Oct 11, 2023

rayy40 commented Oct 13, 2023

ainouzgali left a comment

ainouzgali Oct 15, 2023

ainouzgali Oct 15, 2023

rayy40 Oct 16, 2023

ainouzgali Oct 16, 2023 •

edited

Loading

rayy40 Oct 16, 2023

ainouzgali commented Oct 15, 2023

rayy40 commented Oct 16, 2023

ainouzgali commented Oct 16, 2023

rayy40 commented Oct 16, 2023

rayy40 commented Oct 16, 2023

ainouzgali left a comment

rayy40 commented Oct 17, 2023 •

edited

Loading

ainouzgali left a comment

fix: Added Warning and Error icons to Worlflow settings component #4413

fix: Added Warning and Error icons to Worlflow settings component #4413

Conversation

rayy40 commented Oct 4, 2023 • edited Loading

What change does this PR introduce?

Why was this change needed?

Screenshots

davidsoderberg commented Oct 5, 2023

rayy40 commented Oct 5, 2023 • edited Loading

davidsoderberg commented Oct 5, 2023

davidsoderberg commented Oct 5, 2023

rayy40 commented Oct 5, 2023

davidsoderberg commented Oct 6, 2023

davidsoderberg Oct 6, 2023

Choose a reason for hiding this comment

rayy40 Oct 6, 2023

Choose a reason for hiding this comment

davidsoderberg Oct 6, 2023 • edited Loading

Choose a reason for hiding this comment

rayy40 Oct 6, 2023

Choose a reason for hiding this comment

rayy40 commented Oct 6, 2023

rayy40 commented Oct 8, 2023

davidsoderberg commented Oct 9, 2023 • edited Loading

rayy40 commented Oct 9, 2023

davidsoderberg commented Oct 9, 2023

rayy40 commented Oct 9, 2023

rayy40 commented Oct 11, 2023

LetItRock commented Oct 11, 2023

rayy40 commented Oct 13, 2023

ainouzgali left a comment

Choose a reason for hiding this comment

ainouzgali Oct 15, 2023

Choose a reason for hiding this comment

ainouzgali Oct 15, 2023

Choose a reason for hiding this comment

rayy40 Oct 16, 2023

Choose a reason for hiding this comment

ainouzgali Oct 16, 2023 • edited Loading

Choose a reason for hiding this comment

rayy40 Oct 16, 2023

Choose a reason for hiding this comment

ainouzgali commented Oct 15, 2023

rayy40 commented Oct 16, 2023

ainouzgali commented Oct 16, 2023

rayy40 commented Oct 16, 2023

rayy40 commented Oct 16, 2023

ainouzgali left a comment

Choose a reason for hiding this comment

rayy40 commented Oct 17, 2023 • edited Loading

ainouzgali left a comment

Choose a reason for hiding this comment

rayy40 commented Oct 4, 2023 •

edited

Loading

rayy40 commented Oct 5, 2023 •

edited

Loading

davidsoderberg Oct 6, 2023 •

edited

Loading

davidsoderberg commented Oct 9, 2023 •

edited

Loading

ainouzgali Oct 16, 2023 •

edited

Loading

rayy40 commented Oct 17, 2023 •

edited

Loading