-
Notifications
You must be signed in to change notification settings - Fork 54
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: Revamp UI of Global Notifictaion Configuration #2318
base: develop
Are you sure you want to change the base?
Conversation
...Configurations/ClustersAndEnvironments/ClusterEnvironmentDrawer/ClusterEnvironmentDrawer.tsx
Outdated
Show resolved
Hide resolved
…-configuration-code-refactoring
Quality Gate passedIssues Measures |
feat: css fixes
…ation-code-refactoring feat: notification configuration code refactoring
…tion-test-case chore: Notification test case id
Quality Gate passedIssues Measures |
setState({ ...state, confirmation: false }) | ||
} catch (serverError) { | ||
if (serverError instanceof ServerErrors && serverError.code === 500) { | ||
setState({ ...state, showCannotDeleteDialogModal: true }) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please remove unneccessary setState
return this.state.webhookConfig | ||
} | ||
return this.state.smtpConfig | ||
if (state.isLoading) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please remove this duplicated line
type="button" | ||
data-testid={`${tab.link}-tab-title`} | ||
className={`tab-group__tab dc__unset-button-styles flexbox dc__gap-1 dc__hover-text-n90 dc__gap-6 px-10 py-4 fw-6 ${index === 0 ? 'dc__left-radius-4 ' : ''} | ||
${index === getConfigurationTabTextWithIcon().length - 1 ? 'dc__right-radius-4' : ''} ${activeTab === tab.link ? 'bcn-1 fw-6 cn-9' : 'bcn-0'} cn-7`} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cn-7
should be inside the else case of isActive
const awsRegionListParsed = awsRegionList | ||
.sort((a, b) => stringComparatorBySortOrder(a.name, b.name)) | ||
.map((region) => ({ label: region.name, value: region.value })) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be outside the component
case ConfigurationsTabTypes.WEBHOOK: | ||
return <Webhook className={`icon-dim-${size}`} /> | ||
default: | ||
return SES |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fix the default case
export const renderText = (text: string, isLink: boolean = false, linkTo?: () => void, dataTestId?: string) => ( | ||
<Tooltip content={text} placement="bottom" showOnTruncate={!!text} className="mxh-210 dc__hscroll" interactive> | ||
{isLink ? ( | ||
<button |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be the new Button
component.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With the button component, first letter of the text change to uppercase that's why avoided
return { isValid: true, message: '' } | ||
} | ||
|
||
export const getFormValidated = (isFormValid: FormValidation, fromEmail?: string): boolean => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please verify if this is required
isLoading: false, | ||
isError: true, | ||
})) | ||
setFormValid({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this should be inside the validateForm method
font-weight: inherit; | ||
} | ||
|
||
.ses-config-table__email { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove these from component
color: var(--N900); | ||
font-weight: inherit; | ||
} | ||
.configuration-tab__table-row:hover .slack-config-table__action, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should use visible-hover rather that creating all these
} | ||
}, [smtpConfigId, shouldBeDefault]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why shouldBeDefault
is a dependency?
event.preventDefault() | ||
this.saveWebhookConfig() | ||
} | ||
if (!getAllFieldsValidated()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This code seems duplicated
Description
feat: Revamp UI of Global Notifictaion Configuration
//cards are wrapped in extra div
Fixes # (issue)
Type of change
Checklist: