Page MenuHomePhabricator

[web] Add logged in section
ClosedPublic

Authored by tomek on Mar 2 2022, 7:03 AM.
Tags
None
Referenced Files
F3295951: D3321.diff
Sun, Nov 17, 12:03 AM
Unknown Object (File)
Wed, Nov 6, 7:02 PM
Unknown Object (File)
Tue, Nov 5, 10:11 PM
Unknown Object (File)
Mon, Oct 21, 2:42 AM
Unknown Object (File)
Oct 14 2024, 3:07 AM
Unknown Object (File)
Oct 14 2024, 3:07 AM
Unknown Object (File)
Oct 14 2024, 3:07 AM
Unknown Object (File)
Oct 14 2024, 3:07 AM

Details

Summary
Test Plan

Render the page instead of Calendar and check if it matches the design and if current username was displayed

Diff Detail

Repository
rCOMM Comm
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

tomek edited the summary of this revision. (Show Details)
tomek edited the summary of this revision. (Show Details)
tomek requested review of this revision.Mar 2 2022, 7:08 AM
tomek planned changes to this revision.Mar 2 2022, 7:51 AM
web/settings/account-settings.css
28–31 ↗(On Diff #10024)

We're settings these so that username is truncated. Setting the color is necessary here, as the color of ellipsis is taken from parent instead of the child that is truncated (we want for the ellipsis to have the same color as the username).

benschac added inline comments.
web/settings/account-settings.css
13 ↗(On Diff #10027)

I like to avoid margin as much as possible. Could we get the same result using padding or flex-box?

23 ↗(On Diff #10027)

nit: could padding-bottom: 24px work?

web/settings/account-settings.react.js
11 ↗(On Diff #10027)

It's wild that this screen would show up in a logged out state and force us to write this conditional.

22 ↗(On Diff #10027)

doing a git grep of the project we don't normally add strings like this `{'Logged in as '} . Why do it this way here?

This revision now requires changes to proceed.Mar 2 2022, 8:40 AM
web/settings/account-settings.css
13 ↗(On Diff #10027)

Ok, will change that

23 ↗(On Diff #10027)

No, as the border is rendered between the padding and the margin. We can use padding top though.

web/settings/account-settings.react.js
11 ↗(On Diff #10027)

This screen shouldn't be visible for logged out users... We can replace this conditional with invariant, but I prefer avoiding them because they caused a lot of issues in the past. We can also just render null (or some message) in case of the user being logged out, which sounds like a reasonable solution here.

22 ↗(On Diff #10027)

This string ends with a space, and if you search for '} then there are a couple of usages. If this notation is confusing I can use non-breaking space (which might be a better fit here).

Use paddings and do not render logged out state

benschac added inline comments.
web/settings/account-settings.react.js
22 ↗(On Diff #10027)

that makes sense!

This revision is now accepted and ready to land.Mar 4 2022, 8:12 AM
tomek marked 4 inline comments as done.

Rebase

This revision was automatically updated to reflect the committed changes.