upcoming: [DI - 22836] - Added AddNotificationChannel component #11511
Conversation
…relevant types,schemas
santoshp210-akamai
requested review from
carrillo-erik and
pmakode-akamai
and removed request for
a team
|
Note Currently once the Notification Channel is selected and Submit button is pressed in the Add Notification Channel Drawer, it does not reflect the saved Channel on the form. That is part of a different component that will added as part of subsequent PR. Thank you! |
|
Coverage Report: ✅ |
| <Box paddingTop={2}> | ||
| <Grid container> | ||
| <Grid item md={1} sm={1} xs={2}> | ||
| <Typography variant="h3">To:</Typography> |
There was a problem hiding this comment.
When selecting Email and Channel, the To: label appears empty. Is this intentional, or should a placeholder or dynamic value be displayed here for clarity?
There was a problem hiding this comment.
There is supposed to be another component that shows the details. That'll be part of next PR , we couldn't add it because of the line count.
There was a problem hiding this comment.
The To: label is empty, which could be confusing. Since the full details will be part of the next PR, would it make sense to add both the To: label and its details together then? This would ensure the UI is complete and functional, without leaving any incomplete or ambiguous elements in this PR. I defer to @jaalah-akamai and others for input
There was a problem hiding this comment.
Yeah, that makes sense. I will take a look if it's possible. If others agree then I will make that change
| <Box mt={1}> | ||
| <Button | ||
| buttonType="outlined" | ||
| onClick={onAddNotifications} | ||
| size="medium" | ||
| > | ||
| Add notification channel | ||
| </Button> | ||
| </Box> |
There was a problem hiding this comment.
The top spacing for the Add Notification Channel button seems smaller and inconsistent with the Add Metric button. Can we adjust the spacing to make them consistent?
There was a problem hiding this comment.
Same thing here, There is supposed to be a different section, but The button is part of another component which will be in the next PR so to test the drawer functionality, I put it as placeholder for this PR.
Cloud Manager UI test results🔺 3 failing tests on test run #5 ↗︎
Details
TroubleshootingUse this command to re-run the failing tests: yarn cy:run -s "cypress/e2e/core/linodes/smoke-linode-landing-table.spec.ts,cypress/e2e/core/linodes/clone-linode.spec.ts,cypress/e2e/core/cloudpulse/cloudpulse-navigation.spec.ts" |
|||||||||||||||||||||||
| {openAddNotification && ( | ||
| <Drawer | ||
| onClose={onExitNotifications} | ||
| open={openAddNotification} | ||
| title="Add Notification Channel" | ||
| > | ||
| <AddNotificationChannel | ||
| isNotificationChannelsError={false} | ||
| isNotificationChannelsLoading={false} | ||
| onCancel={onExitNotifications} | ||
| onSubmitAddNotification={onSubmitAddNotification} | ||
| templateData={notificationChannelFactory.buildList(2)} | ||
| /> | ||
| </Drawer> | ||
| )} |
There was a problem hiding this comment.
Let's remove this conditional. It breaks the drawer open/close animation
| {openAddNotification && ( | |
| <Drawer | |
| onClose={onExitNotifications} | |
| open={openAddNotification} | |
| title="Add Notification Channel" | |
| > | |
| <AddNotificationChannel | |
| isNotificationChannelsError={false} | |
| isNotificationChannelsLoading={false} | |
| onCancel={onExitNotifications} | |
| onSubmitAddNotification={onSubmitAddNotification} | |
| templateData={notificationChannelFactory.buildList(2)} | |
| /> | |
| </Drawer> | |
| )} | |
| <Drawer | |
| onClose={onExitNotifications} | |
| open={openAddNotification} | |
| title="Add Notification Channel" | |
| > | |
| <AddNotificationChannel | |
| isNotificationChannelsError={false} | |
| isNotificationChannelsLoading={false} | |
| onCancel={onExitNotifications} | |
| onSubmitAddNotification={onSubmitAddNotification} | |
| templateData={notificationChannelFactory.buildList(2)} | |
| /> | |
| </Drawer> |
The Drawer component should be smart enough to not render its children when open is false so performance should still be okay
Description 📝
Added the AddNotificationChannel Drawer component to the Create Alert form
Changes 🔄
Target release date 🗓️
Please specify a release date (and environment, if applicable) to guarantee timely review of this PR. If exact date is not known, please approximate and update it as needed.
Preview 📷
Include a screenshot or screen recording of the change.
🔒 Use the Mask Sensitive Data setting for security.
💡 Use
<video src="" />tag when including recordings in table.How to test 🧪
Prerequisites
Add Notification Channelbutton, clicking on that should show the Add Notification Channel DrawerReproduction steps
Verification steps
Author Checklists
As an Author, to speed up the review process, I considered 🤔
👀 Doing a self review
❔ Our contribution guidelines
🤏 Splitting feature into small PRs
➕ Adding a changeset
🧪 Providing/improving test coverage
🔐 Removing all sensitive information from the code and PR description
🚩 Using a feature flag to protect the release
👣 Providing comprehensive reproduction steps
📑 Providing or updating our documentation
🕛 Scheduling a pair reviewing session
📱 Providing mobile support
♿ Providing accessibility support
As an Author, before moving this PR from Draft to Open, I confirmed ✅