Emails page redesign #15778

artemruts · 2020-11-14T04:04:01Z

Reworked Setting/Emails page, removed FilteredConnection and hopefully simplified a few things.
There's a "Set primary address" select now, ty @ryanslade.
A 80% of the time I spend recalling React.

CSS:

Maybe select element can have fixed(-ish) width?
Replace filtered-connection styles, what approach should I take?
Not sure why modal looks that way, why did we set width: 32 rem?

@quinnkeast some of the styles are definitely not 1 to 1 as in Figma, which elements stand out to you the most?

codecov · 2020-11-14T04:18:08Z

Codecov Report

Merging #15778 (4da6551) into main (e6ba3e5) will decrease coverage by 0.00%.
The diff coverage is 6.72%.

@@            Coverage Diff             @@
##             main   #15778      +/-   ##
==========================================
- Coverage   52.97%   52.97%   -0.01%     
==========================================
  Files        1649     1651       +2     
  Lines       82442    82452      +10     
  Branches     7413     7334      -79     
==========================================
+ Hits        43671    43676       +5     
- Misses      34928    34932       +4     
- Partials     3843     3844       +1

Flag	Coverage Δ
go	`52.56% <ø> (-0.01%)`	⬇️
integration	`29.10% <0.00%> (-0.05%)`	⬇️
storybook	`28.00% <22.22%> (-0.01%)`	⬇️
typescript	`53.95% <6.72%> (+<0.01%)`	⬆️
unit	`36.02% <6.72%> (+0.04%)`	⬆️

Impacted Files	Coverage Δ
.../web/src/user/settings/emails/AddUserEmailForm.tsx	`0.00% <0.00%> (ø)`
...c/user/settings/emails/SetUserPrimaryEmailForm.tsx	`0.00% <0.00%> (ø)`
client/web/src/user/settings/emails/UserEmail.tsx	`0.00% <0.00%> (ø)`
...rc/user/settings/emails/UserSettingsEmailsPage.tsx	`0.00% <0.00%> (ø)`
client/shared/src/util/useInputValidation.ts	`94.00% <88.88%> (-1.24%)`	⬇️
cmd/repo-updater/repos/syncer.go	`76.92% <0.00%> (-0.54%)`	⬇️

client/web/src/user/settings/emails/UserEmail.tsx

client/web/src/user/settings/emails/UserSettingsEmailsPage.tsx

quinnkeast · 2020-11-18T09:47:19Z

I've created a visual QA in Figma here:

https://www.figma.com/file/zXvqCASHDzmvKbrAl1EVsd/Public-and-Private-Repos-for-Users-on-Cloud?node-id=291%3A228

I don't know if this is the best format, but let me know if it works or if I should make any adjustments to how I share this QA feedback!

Re: confirmation modal, I don't believe we need to show the confirmation modal. Only the non-primary email can be removed, and it's easy enough to add it back that we don't need the extra confirmation step.

felixfbecker

I'm really excited to see these pages cleaned up, great job!
I left a lot of comments with patterns and links to docs that you could not have been aware of yet so I hope you're not intimidated! I'm happy to go over this in our buddy sync later :)

client/web/src/user/settings/emails/SetUserPrimaryEmailForm.tsx

client/web/src/user/settings/emails/UserEmail.tsx

client/web/src/user/settings/emails/UserSettingsEmailsPage.tsx

client/web/src/user/settings/emails/UserEmail.tsx

client/web/src/user/settings/emails/AddUserEmailForm.tsx

…mailForm (#16065)

client/web/src/user/settings/emails/AddUserEmailForm.tsx

artemruts · 2020-11-24T21:22:08Z

@quinnkeast would it be possible to ship this UI without the resend email verification link?
I'll file a separate issue and pair with somebody on this.

Also can you please check this UI one more time? 🙇

quinnkeast · 2020-11-25T09:29:58Z

@quinnkeast would it be possible to ship this UI without the resend email verification link?
I'll file a separate issue and pair with somebody on this.

Yes, that sounds fine! Let me know if I can help with the issue.

Also can you please check this UI one more time? 🙇

For sure. Two last bits of feedback:

The placeholder on the Email input isn't needed, and should be removed.

The external corners of the email list should have a 4px corner radius, like so:

client/web/src/user/settings/emails/UserSettingsEmailsPage.tsx

client/web/src/user/settings/emails/UserSettingsEmailsPage.scss

artemruts · 2020-11-25T17:22:41Z

client/web/src/user/settings/emails/UserSettingsEmailsPage.scss

+    .form-inline input:not(.is-invalid),
+    .form-inline input:not(.is-valid) {
+        padding-right: 2rem;
+    }
+    .loader-input {
+        &__spinner {
+            right: 1rem;
+        }
+    }


These changes are to make LoaderInput component look good when parent element has form-inline class.
By default it looked like this:

That seems like a bug in LoaderInput, could we fix that generally for the LoaderInput instead of just here? cc @tjkandala

Fixed in latest commit by moving mr-sm-2 up from the <input> to <LoaderInput>.

The width of <LoaderInput>s container <div> was inflated by the margin, so right wasn't big enough on the absolutely positioned spinner.

Before

After

@felixfbecker I moved some imports around (brought loading spinner to branded), can you take a look to make sure I didn't miss anything?

client/web/src/user/settings/emails/UserSettingsEmailsPage.scss

tjkandala · 2020-11-27T19:52:32Z

Pushed a quick commit to fix a UI state issue when emails are deleted in quick succession. We were filtering on emails at the time that the user clicked "remove", so when the mutation promise resolved, there was a chance that we added deleted emails back to the array. Now, we use a setState callback so we're always passed the latest emails.

Email deletion before

Email deletion after

Also ensures that users can't click "Add" when the email is not valid

felixfbecker · 2020-11-27T19:54:05Z

That's the kind of bug refetching avoids :)

artemruts · 2020-11-27T20:06:21Z

@tjkandala thanks for UI fixes 🙇

client/shared/src/util/useInputValidation.ts

artemruts · 2020-11-30T21:49:28Z

client/shared/src/util/useInputValidation.test.ts

@@ -1,8 +1,11 @@
+/* eslint-disable @typescript-eslint/no-floating-promises */


There's a false-positive with act calls in @testing-library/react-hooks

client/shared/src/util/useInputValidation.test.ts

tsenart

LGTM visually, trust you can coordinate with the @sourcegraph/web team on any follow up changes they want to infrastructure, shared components, etc. We should not block this feature on those.

felixfbecker

Feel free to address comments at your discretion and merge

client/web/src/user/settings/emails/SetUserPrimaryEmailForm.tsx

client/web/src/user/settings/emails/UserEmail.tsx

felixfbecker · 2020-12-01T13:47:39Z

client/web/src/user/settings/emails/UserSettingsEmailsPage.scss

+    .form-inline input:not(.is-invalid),
+    .form-inline input:not(.is-valid) {
+        padding-right: 2rem;
+    }


question: What is this CSS still needed for?

It's still used to prevent the input element from "jumping" when spinner/valid/invalid icons appear

Shouldn't that apply to all inputs then, not just this particular input? I.e. this style should live in global-styles/?

client/web/src/user/settings/emails/UserSettingsEmailsPage.tsx

Co-authored-by: Felix Becker <felix.b@outlook.com>

…cegraph/sourcegraph into cloud-emails-redesign-15409

artemruts added 4 commits November 13, 2020 11:10

cleanup before mergin main

a0c8424

Merge branch 'main' into cloud-emails-redesign-15409

103c8aa

Beta version

5299228

Remove unused imports

476fcef

Ready for comments

91c4201

artemruts commented Nov 18, 2020

View reviewed changes

Formatting and lint fixes

14c0772

artemruts requested review from felixfbecker and quinnkeast November 18, 2020 06:16

artemruts marked this pull request as ready for review November 18, 2020 06:17

felixfbecker reviewed Nov 18, 2020

View reviewed changes

client/web/src/user/settings/emails/AddUserEmailForm.tsx Outdated Show resolved Hide resolved

artemruts added 5 commits November 19, 2020 00:56

Addressing comments - 1

92849c4

Addressing comments - 2

1b65ca7

Addressing comments - 3

01676bc

Minor tweaks to the select

dc785a2

Adjust padding

ee20854

artemruts requested a review from felixfbecker November 23, 2020 03:03

artemruts and others added 2 commits November 23, 2020 12:57

Simplifying code

09863eb

Allow consumers to override useInputValidation state, use in AddUserE…

4330f6a

…mailForm (#16065)

artemruts commented Nov 24, 2020

View reviewed changes

client/web/src/user/settings/emails/AddUserEmailForm.tsx Outdated Show resolved Hide resolved

felixfbecker reviewed Nov 25, 2020

View reviewed changes

client/web/src/user/settings/emails/UserSettingsEmailsPage.tsx Outdated Show resolved Hide resolved

felixfbecker reviewed Nov 25, 2020

View reviewed changes

client/web/src/user/settings/emails/UserSettingsEmailsPage.scss Show resolved Hide resolved

Remove hook changes and simplify css

391ca02

artemruts force-pushed the cloud-emails-redesign-15409 branch from a8a9ff0 to 391ca02 Compare November 25, 2020 17:05

Replace custom css with utils

9f7f08f

artemruts commented Nov 25, 2020

View reviewed changes

client/web/src/user/settings/emails/UserSettingsEmailsPage.scss Outdated Show resolved Hide resolved

artemruts and others added 2 commits November 25, 2020 15:50

Remove unneeded styles

3150b9e

fix stale onEmailRemove

f0ef192

tjkandala added 3 commits November 27, 2020 16:16

fix loader input spinner

8bd340b

Merge branch 'main' into cloud-emails-redesign-15409

02ebdb3

dedup storybook import

3ed6dda

artemruts commented Nov 30, 2020

View reviewed changes

client/shared/src/util/useInputValidation.ts Show resolved Hide resolved

tjkandala and others added 2 commits November 30, 2020 10:48

fix LoaderInput story

633650b

Attempt to cast types

edabd7c

artemruts force-pushed the cloud-emails-redesign-15409 branch from 0de13a3 to edabd7c Compare November 30, 2020 21:41

artemruts commented Nov 30, 2020

View reviewed changes

client/shared/src/util/useInputValidation.test.ts Outdated Show resolved Hide resolved

artemruts requested a review from felixfbecker November 30, 2020 21:50

tsenart added the team/cloud label Dec 1, 2020

tsenart added this to the Cloud 2020-11-18 milestone Dec 1, 2020

This was referenced Dec 1, 2020

WIP: Cloud Milestone 3 Tracking issue #15610

Closed

Cloud 2020-11-18 Tracking issue #15753

Closed

quinnkeast approved these changes Dec 1, 2020

View reviewed changes

tsenart approved these changes Dec 1, 2020

View reviewed changes

felixfbecker approved these changes Dec 1, 2020

View reviewed changes

artemruts and others added 3 commits December 1, 2020 10:39

Update client/shared/src/util/useInputValidation.test.ts

6b6aa09

Co-authored-by: Felix Becker <felix.b@outlook.com>

Addressing feedback

8760033

Merge branch 'cloud-emails-redesign-15409' of https://github.com/sour…

4da6551

…cegraph/sourcegraph into cloud-emails-redesign-15409

artemruts merged commit a5fba26 into main Dec 1, 2020

artemruts deleted the cloud-emails-redesign-15409 branch December 1, 2020 18:25

sourcegraph-bot mentioned this pull request Dec 1, 2020

Update Sourcegraph Docker insiders images sourcegraph/deploy-sourcegraph#1395

Merged

1 task

		@@ -1,8 +1,11 @@
		/* eslint-disable @typescript-eslint/no-floating-promises */

Emails page redesign #15778

Emails page redesign #15778

Uh oh!

Conversation

artemruts commented Nov 14, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

CSS:

Uh oh!

codecov bot commented Nov 14, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

quinnkeast commented Nov 18, 2020

Uh oh!

felixfbecker left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

artemruts commented Nov 24, 2020

Uh oh!

quinnkeast commented Nov 25, 2020

Uh oh!

Uh oh!

Uh oh!

artemruts Nov 25, 2020

Choose a reason for hiding this comment

Uh oh!

felixfbecker Nov 25, 2020

Choose a reason for hiding this comment

Uh oh!

tjkandala Nov 27, 2020

Choose a reason for hiding this comment

Before

After

Uh oh!

Uh oh!

tjkandala commented Nov 27, 2020

Uh oh!

felixfbecker commented Nov 27, 2020

Uh oh!

artemruts commented Nov 27, 2020

Uh oh!

Uh oh!

artemruts Nov 30, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

tsenart left a comment

Choose a reason for hiding this comment

Uh oh!

felixfbecker left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

felixfbecker Dec 1, 2020

Choose a reason for hiding this comment

Uh oh!

artemruts Dec 1, 2020

Choose a reason for hiding this comment

Uh oh!

felixfbecker Dec 1, 2020

Choose a reason for hiding this comment

artemruts commented Nov 14, 2020 •

edited

Loading

codecov bot commented Nov 14, 2020 •

edited

Loading