-
Notifications
You must be signed in to change notification settings - Fork 367
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
upcoming: [M3-9008] Improve Close Account UI #11469
Conversation
onChange: (value: string) => void; | ||
textFieldStyle?: React.CSSProperties; | ||
title?: string; | ||
typographyStyle?: React.CSSProperties; | ||
typographyStyleSx?: SxProps<Theme>; |
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.
typographyStyle
must've been old. typographyStyleSx
is a lot better.
confirmText: '', | ||
services: false, | ||
users: false, | ||
}); |
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.
Tossed validation into a state object - RHF seemed overkill
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.
I will follow up with a PR to convert this to RHF afterwards
const isTextConfirmationValid = deleteAccount.confirmText === entity.name; | ||
const isCloseAccount = entity.subType === 'CloseAccount'; | ||
const isCloseAccountValid = | ||
!isCloseAccount || (deleteAccount.services && deleteAccount.users); |
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.
Hoping to provide some better readability with these
hideInstructions: false, | ||
placeholder: '', | ||
}; | ||
}; |
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.
Noticed these props were all based on the CloseAccount
subtype
<ActionsPanel | ||
{...getButtonProps()} | ||
reversePrimaryButtonPosition | ||
style={{ padding: 0 }} |
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.
I kept this, but could probably be moved to maybe?
outlined: 'outlined', | ||
primary: 'contained', | ||
secondary: 'contained', | ||
} as const; |
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.
The maps will help if we need more variants or colors
spacingBottom !== undefined ? `${spacingBottom}px` : theme.spacing(3), | ||
marginLeft: spacingLeft !== undefined ? `${spacingLeft}px` : 0, | ||
marginTop: spacingTop !== undefined ? `${spacingTop}px` : 0, | ||
sx, |
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 sx
prop did not accept styles passed down that used theme
. So I fixed this 🐛
interface ButtonPropsVariantOverrides { | ||
loading: 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.
I don't forsee us doing this much, but we may once we start going deep with CDS
Coverage Report: ✅ |
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 type-to-confirm disabled, there's no way to close the account and no explanation for why the button is disabled.
I thought in the past, this field was independent of the TTC profile setting but looks like that's not the case anymore 🤔. I think we should always show the TTC input for the close account dialog regardless of user setting
Thanks @hana-akamai should be enforced now |
Cloud Manager UI test results🎉 0 passing tests on test run #9 ↗︎
|
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.
Looks like there's some failing unit tests
Can we also add a test to check that the TTC input is still displayed for account cancellation when the user has the setting enabled?
@hana-akamai It saw that it had to do with the new |
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.
packages/manager/cypress/e2e/core/account/account-cancellation.spec.ts
Outdated
Show resolved
Hide resolved
packages/manager/cypress/e2e/core/account/account-cancellation.spec.ts
Outdated
Show resolved
Hide resolved
packages/manager/cypress/e2e/core/account/account-cancellation.spec.ts
Outdated
Show resolved
Hide resolved
packages/manager/src/components/TypeToConfirmDialog/TypeToConfirmDialog.tsx
Outdated
Show resolved
Hide resolved
….spec.ts Co-authored-by: Connie Liu <139280159+coliu-akamai@users.noreply.github.com>
….spec.ts Co-authored-by: Connie Liu <139280159+coliu-akamai@users.noreply.github.com>
….spec.ts Co-authored-by: Connie Liu <139280159+coliu-akamai@users.noreply.github.com>
@coliu-akamai Sorry for not pointing that out, the fields will span full width now by default |
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.
Looks great, nice to have these extra safeguards! Test improvements look great too, thanks @jaalah-akamai!
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.
Looks good! Can we merge develop in and make sure tests are passing before merging?
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.
dialog text field widths looking good for the dialogs that were pushed up ✅ (+ potential follow up pr for the rest)
thank you!
* upcoming: [M3-9008] Improve Close Account UI * Add changesets * Update e2e test to cover parent/child * More cleanup * Change warning to error and update variants/colors * Enforce type-to-confirm for close account * Small revert * Revision * Text updates to title, warning, comment blurb * Expand comments section full width - allow this prop to be set * Fix unit tests due to usePreference updates - remove loading variant * Update packages/manager/cypress/e2e/core/account/account-cancellation.spec.ts Co-authored-by: Connie Liu <139280159+coliu-akamai@users.noreply.github.com> * Update packages/manager/cypress/e2e/core/account/account-cancellation.spec.ts Co-authored-by: Connie Liu <139280159+coliu-akamai@users.noreply.github.com> * Update packages/manager/cypress/e2e/core/account/account-cancellation.spec.ts Co-authored-by: Connie Liu <139280159+coliu-akamai@users.noreply.github.com> * Update state variable name @coliu-akamai * Allow expand to be passed for deletionDialogs * Expand all delete type-to-confirms @coliu-akamai * more deletion dialogs * Few more dialogs --------- Co-authored-by: Jaalah Ramos <jaalah.ramos@gmail.com> Co-authored-by: Connie Liu <139280159+coliu-akamai@users.noreply.github.com>
Description 📝
We've updated our account closure confirmation dialog to clearly explain this permanent action. This includes clearer warnings, confirmation checkboxes for service/user deletion, and updated the Close Account button styling.
Changes 🔄
reversePrimaryButtonPosition
prop toActionsPanel
component to support the new design's button layout (primary left, secondary right)email
instead ofusername
color="warning"
Button
component to allow for newcolor
and added some maps for scalabilityloading
variant toMuiButton
theme 🎨Target release date 🗓️
01/14/2025 (Next Release)
Preview 📷
How to test 🧪
Prerequisites
Reproduction steps
/account/settings
Close Account
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 ✅