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): workflow settings - providers warnings were cut off #5300

Merged
merged 1 commit into from
Mar 14, 2024

Conversation

LetItRock
Copy link
Contributor

What change does this PR introduce?

Fixed the issue in the Workflow Settings - Providers tab, that the channel "warning banners" were cut off.

Why was this change needed?

Other information (Screenshots)

Screen.Recording.2024-03-13.at.18.59.54.mov

@LetItRock LetItRock self-assigned this Mar 13, 2024
Copy link

linear bot commented Mar 13, 2024

Copy link

netlify bot commented Mar 13, 2024

Deploy Preview for dev-web-novu failed.

Name Link
🔨 Latest commit 7196f3d
🔍 Latest deploy log https://app.netlify.com/sites/dev-web-novu/deploys/65f1eb503202d00008b25498

Comment on lines +81 to +87
const providersForTheCurrentEnvironment = useMemo(
() =>
channelProviders.filter((provider) => provider.connected && provider.environmentId === currentEnvironment?._id),
[channelProviders, currentEnvironment?._id]
);
const hasProviders = providersForTheCurrentEnvironment.length > 0;
const hasNovuProvider = providersForTheCurrentEnvironment.length === 1 && containsNovuProvider;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Under the Providers tab, we are showing all the integrations from the current environment, so the one change that I introduced is the logic to show the warnings is based on the assumption of the current environment.

padding: '7.5px 15px',
}}
variant={providers.filter((provider) => provider.connected).length > 0 ? 'outline' : 'gradient'}
<ListProvidersContainer>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I could spend probably more time refactoring it, but I don't think it's worth it and to me, the current state is better than what we had before.

>
<div
style={{
marginBottom: -28,
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 reason why it was cut off

Copy link
Contributor

@antonjoel82 antonjoel82 left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

Copy link
Contributor

Choose a reason for hiding this comment

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

💭 thought: ‏This could be a great opportunity to use panda instead! It should simplify some of this CSS

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought about it, but wasn't sure how should I map the current colors to the new color palette.

Comment on lines +74 to +85
const containsNovuProvider = useMemo(
() =>
NOVU_SMS_EMAIL_PROVIDERS.some(
(providerId) => providerId === channelProviders.find((provider) => provider.connected)?.providerId
),
[channelProviders]
);
const providersForTheCurrentEnvironment = useMemo(
() =>
channelProviders.filter((provider) => provider.connected && provider.environmentId === currentEnvironment?._id),
[channelProviders, currentEnvironment?._id]
);
Copy link
Contributor

Choose a reason for hiding this comment

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

🎯 suggestion: These are more or less "selectors," and could be written and reused as standalone functions outside of this component and file. This will help us from re-inventing the wheel because I imagine we have an instance or two of this implemented elsewhere in the app

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 don't because they are specific to this case, but we can extract it into more reusable code.

@LetItRock LetItRock merged commit 405b147 into next Mar 14, 2024
24 of 28 checks passed
@LetItRock LetItRock deleted the nv-3283-providers-warning-cut-off branch March 14, 2024 08:48
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