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

[dashboard] account deletion #3551

Merged
merged 1 commit into from
Mar 23, 2021
Merged

[dashboard] account deletion #3551

merged 1 commit into from
Mar 23, 2021

Conversation

svenefftinge
Copy link
Member

@svenefftinge svenefftinge commented Mar 23, 2021

Fixes #3395

@svenefftinge svenefftinge force-pushed the se/profile-page branch 2 times, most recently from 7cd9a55 to bc6d6dc Compare March 23, 2021 09:36
@svenefftinge svenefftinge requested a review from gtsiolis March 23, 2021 09:37
@svenefftinge svenefftinge marked this pull request as ready for review March 23, 2021 09:37

const deleteAccount = async () => {
await getGitpodService().server.deleteAccount();
//TODO we are sad to see you go view
Copy link
Member Author

Choose a reason for hiding this comment

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

@gtsiolis I made it so that users get redirected to the home page.
We should think about something that helps users reconsidering their decision.
That said probably something to postpone to after this milestone.

Copy link
Contributor

Choose a reason for hiding this comment

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

Current flow looks good but yes, we could improve this. Some visual feedback (see alert component) on the login page that the account has been deleted could be a starting point. I'd also vote for leaving this out of the scope of this milestone. Added #3554 as a follow-up issue.

@svenefftinge
Copy link
Member Author

@AlexTugarev After I have deleted my account I can no longer login. Any idea why that is?

const deleteAccount = async () => {
await getGitpodService().server.deleteAccount();
//TODO we are sad to see you go view
document.location.href = 'https://www.gitpod.io';
Copy link
Member

Choose a reason for hiding this comment

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

independent of the TODO, I think we should use the website URL from branding.

Copy link
Member

Choose a reason for hiding this comment

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

also this #3551 (comment)

Copy link
Member Author

Choose a reason for hiding this comment

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

We don't support "branding" anymore, just saas and self-hosted. In both cases users could land on our website.
But I changed this now to just go to logout.

@AlexTugarev
Copy link
Member

@AlexTugarev After I have deleted my account I can no longer login. Any idea why that is?

yes. the session isn't destroyed on the API call.

we need to redirect to /api/logout?returnTo=ExitURL

@svenefftinge svenefftinge force-pushed the se/profile-page branch 2 times, most recently from 455a889 to 22527ee Compare March 23, 2021 09:55
@AlexTugarev
Copy link
Member

AlexTugarev commented Mar 23, 2021

Screen Shot 2021-03-23 at 11 04 48

Rendering the result as placeholder feels odd. The input field already contains email address, so why should I enter it there 🤷🏻‍♂️

@svenefftinge
Copy link
Member Author

svenefftinge commented Mar 23, 2021

Rendering the result as placeholder feels odd. The input field already contains email address, so why should I enter it there 🤷🏻‍♂️

I think that the additional step is a commonly used guard to avoid accidental deletions.

@AlexTugarev
Copy link
Member

@svenefftinge, I got that. The thing is, it looks like the input field is already pre-filled, as if the browser already autofilled it maybe.

Maybe it would help to prefix the placeholder text, e.g.
[ your email is alex@gitpod.io ]

Copy link
Member

@AlexTugarev AlexTugarev left a comment

Choose a reason for hiding this comment

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

works nicely!

@svenefftinge
Copy link
Member Author

@svenefftinge, I got that. The thing is, it looks like the input field is already pre-filled, as if the browser already autofilled it maybe.

Maybe it would help to prefix the placeholder text, e.g.
[ your email is alex@gitpod.io ]

@gtsiolis WDYT?

@gtsiolis
Copy link
Contributor

gtsiolis commented Mar 23, 2021

Looking at this now! 👀

Copy link
Contributor

@gtsiolis gtsiolis left a comment

Choose a reason for hiding this comment

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

Looks good @svenefftinge! Left some minor comments. 🏓


const close = () => setModal(false);
return <div>
<Modal visible={modal} onClose={close}>
<h3 className="pb-2">Delete Account</h3>
<div className="border-t border-b border-gray-200 mt-2 -mx-6 px-6 py-2">
<p className="mt-1 mb-2 text-base">Are you sure you want to delete your account? This action will remove all the data associated with your account in Gitpod and cannot be reversed.</p>
<p className="py-4 text-gray-900 text-base">You are about to permanently delete your account.</p>
<ol className="text-gray-400 text-sm list-outside list-decimal">
Copy link
Contributor

Choose a reason for hiding this comment

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

nitpick: Minor font color shift.

Suggested change
<ol className="text-gray-400 text-sm list-outside list-decimal">
<ol className="text-gray-500 text-sm list-outside list-decimal">


const close = () => setModal(false);
return <div>
<Modal visible={modal} onClose={close}>
<h3 className="pb-2">Delete Account</h3>
<div className="border-t border-b border-gray-200 mt-2 -mx-6 px-6 py-2">
<p className="mt-1 mb-2 text-base">Are you sure you want to delete your account? This action will remove all the data associated with your account in Gitpod and cannot be reversed.</p>
<p className="py-4 text-gray-900 text-base">You are about to permanently delete your account.</p>
<ol className="text-gray-400 text-sm list-outside list-decimal">
Copy link
Contributor

@gtsiolis gtsiolis Mar 23, 2021

Choose a reason for hiding this comment

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

praise: Always fun to see W3C specs going live as browser standards and being available only with a class name! Go list-outside! 🔮


const close = () => setModal(false);
return <div>
<Modal visible={modal} onClose={close}>
<h3 className="pb-2">Delete Account</h3>
<div className="border-t border-b border-gray-200 mt-2 -mx-6 px-6 py-2">
<p className="mt-1 mb-2 text-base">Are you sure you want to delete your account? This action will remove all the data associated with your account in Gitpod and cannot be reversed.</p>
<p className="py-4 text-gray-900 text-base">You are about to permanently delete your account.</p>
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
<p className="py-4 text-gray-900 text-base">You are about to permanently delete your account.</p>
<p className="pb-4 text-gray-900 text-base">You are about to permanently delete your account.</p>

await getGitpodService().server.deleteAccount();
//TODO we are sad to see you go view
document.location.href = gitpodHostUrl.asApiLogout().toString();
};

const close = () => setModal(false);
return <div>
<Modal visible={modal} onClose={close}>
<h3 className="pb-2">Delete Account</h3>
<div className="border-t border-b border-gray-200 mt-2 -mx-6 px-6 py-2">
Copy link
Contributor

Choose a reason for hiding this comment

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

nitpick: Minor padding changes.

Suggested change
<div className="border-t border-b border-gray-200 mt-2 -mx-6 px-6 py-2">
<div className="border-t border-b border-gray-200 mt-2 -mx-6 px-6 py-4">

</div>
<div className="flex justify-end mt-6">
<button className="border-red-900 bg-red-500 hover:bg-red-700" onClick={close}>Delete Account</button>
<button className="text-gray-900 border-gray-100 bg-gray-100 hover:bg-gray-200 hover:border-gray-400" onClick={close}>Cancel</button>
<button className="ml-2 border-red-900 bg-red-500 hover:bg-red-700 disabled:opacity-50" onClick={deleteAccount} disabled={typedEmail !== primaryEmail}>Delete Account</button>
Copy link
Contributor

Choose a reason for hiding this comment

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

question: Can we disable the hover changes when the button is disabled?

</div>
<div className="flex justify-end mt-6">
<button className="border-red-900 bg-red-500 hover:bg-red-700" onClick={close}>Delete Account</button>
<button className="text-gray-900 border-gray-100 bg-gray-100 hover:bg-gray-200 hover:border-gray-400" onClick={close}>Cancel</button>
Copy link
Contributor

Choose a reason for hiding this comment

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

suggestion: Let's remove the background here for the default state and add the font-medium class which is missing also in some other buttons, too.

Suggested change
<button className="text-gray-900 border-gray-100 bg-gray-100 hover:bg-gray-200 hover:border-gray-400" onClick={close}>Cancel</button>
<button className="text-gray-500 border-white bg-white hover:bg-gray-100 hover:border-gray-100 font-medium" onClick={close}>Cancel</button>

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm adding text-base to the default button style

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, wait. Buttons use 14px text. The should be using font-medium, too.

Copy link
Member Author

Choose a reason for hiding this comment

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

Actually, button already had text-sm applied. Is that not right?

Copy link
Contributor

@gtsiolis gtsiolis Mar 23, 2021

Choose a reason for hiding this comment

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

Yes, text-sm is corrent. Font weight (font-medium) is missing. I tried this in #3521 (comment) but didn't work. Maybe I've missed something. ❓

thought: Font weight is not picked up when using @apply in index.css. Maybe this is by design?

<li className="ml-5">Your subscription will be cancelled. If you obtained a Gitpod subscription through the GitHub marketplace, you need to cancel your plan there.</li>
</ol>
<p className="pt-4 pb-2 text-gray-600 text-base font-bold">Type your email to confirm</p>
<input className="border-2 border-gray-400 w-full" type="text" placeholder={primaryEmail} onChange={e => setTypedEmail(e.target.value)}></input>
Copy link
Contributor

Choose a reason for hiding this comment

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

suggestion: Maybe the specs were not clear enough. This shouldn't have any placeholder. 😇 /cc @AlexTugarev

Suggested change
<input className="border-2 border-gray-400 w-full" type="text" placeholder={primaryEmail} onChange={e => setTypedEmail(e.target.value)}></input>
<input className="border-2 border-gray-400 w-full" type="text" onChange={e => setTypedEmail(e.target.value)}></input>

Copy link
Member

Choose a reason for hiding this comment

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

Screen Shot 2021-03-23 at 14 32 48

I'd like to copy the pattern. Others do that in a single line.

Copy link
Contributor

Choose a reason for hiding this comment

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

@AlexTugarev I've cross-linked this conversation to the follow up issue in #3554 (comment).

@@ -65,6 +65,11 @@ module.exports = {
'grayscale': 'grayscale(1)',
},
},
variants: {
extend: {
opacity: ['disabled'],
Copy link
Contributor

Choose a reason for hiding this comment

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

thought: Ideally we should not use opacity for disabled states but different colors, however, let's leave this is out of the scope of this milestone. FWIW, our design specs have not reached this level of maturity yet, it's still missing aspects like tabindex, active, focus state, etc for many elements. Food for thought! 🍔

</div>
<div className="flex justify-end mt-6">
<button className="border-red-900 bg-red-500 hover:bg-red-700" onClick={close}>Delete Account</button>
<button className="text-gray-900 border-gray-100 bg-gray-100 hover:bg-gray-200 hover:border-gray-400" onClick={close}>Cancel</button>
<button className="ml-2 border-red-900 bg-red-500 hover:bg-red-700 disabled:opacity-50" onClick={deleteAccount} disabled={typedEmail !== primaryEmail}>Delete Account</button>
Copy link
Contributor

@gtsiolis gtsiolis Mar 23, 2021

Choose a reason for hiding this comment

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

suggestion: Also, missing the font-medium class.

Suggested change
<button className="ml-2 border-red-900 bg-red-500 hover:bg-red-700 disabled:opacity-50" onClick={deleteAccount} disabled={typedEmail !== primaryEmail}>Delete Account</button>
<button className="ml-2 border-red-900 bg-red-500 hover:bg-red-700 disabled:opacity-50 font-medium" onClick={deleteAccount} disabled={typedEmail !== primaryEmail}>Delete Account</button>

@svenefftinge
Copy link
Member Author

svenefftinge commented Mar 23, 2021

/werft run

👍 started the job as gitpod-build-se-profile-page.8

@svenefftinge svenefftinge merged commit 6e48d21 into main Mar 23, 2021
@svenefftinge svenefftinge deleted the se/profile-page branch March 23, 2021 15:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Dashboard] Account Page
3 participants