Page MenuHomePhabricator

[web] Introduce empty account page
ClosedPublic

Authored by tomek on Mar 2 2022, 6:14 AM.
Tags
None
Referenced Files
Unknown Object (File)
Thu, Dec 19, 4:28 PM
Unknown Object (File)
Wed, Dec 18, 2:25 PM
Unknown Object (File)
Wed, Dec 18, 11:12 AM
Unknown Object (File)
Nov 20 2024, 4:57 PM
Unknown Object (File)
Nov 20 2024, 4:57 PM
Unknown Object (File)
Nov 20 2024, 4:57 PM
Unknown Object (File)
Nov 19 2024, 4:57 AM
Unknown Object (File)
Nov 5 2024, 10:11 PM

Details

Summary

An empty page with only a header. This page is not rendered yet, so there are no visible changes in the app. The rendering will be handled in much later diff.
Designs can be found here: https://www.figma.com/file/a1nkbWgbgjRlrOY9LVurTz/Comm-%2F-Desktop-app?node-id=1272%3A88750

account-settings-1.png (642×888 px, 21 KB)

Test Plan

Display the page instead of e.g. Calendar and check if it is displayed correctly

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

tomek requested review of this revision.Mar 2 2022, 6:22 AM
benschac added inline comments.
web/settings/account-settings.css
2 ↗(On Diff #10015)

The padding in figma is saying 24px. why 40px?

8 ↗(On Diff #10015)

Looking at figma, I don't think this should be semi bold.

Image 2022-03-02 at 11.12.05 AM.jpg (778×1 px, 112 KB)

Also, could the default color be: Shades of Black / 60 that way when a user clicks the link we can apply an active class to the text and give the text a selected color of var(--fg).

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

I think you might be looking at the wrong "My Account" in Figma..

I think it's this one:

82e1-1.png (762×1 px, 684 KB)

Looks good, question inline about the fixed width of the container div

web/settings/account-settings.css
2 ↗(On Diff #10015)
3 ↗(On Diff #10015)

I see that the inner frame on Figma has a width of 456px, but I'm not sure that is necessarily supposed to translate over to the container div having a fixed width of 456px in absolute units?

I think we should probably lay out this component using grid or relative units?

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

Sorry, was looking at the wrong part of the page just looking at the screen shot without context of the entire page. I should have looked at the key word "page" and not assumed it was the sidebar.

This revision is now accepted and ready to land.Mar 2 2022, 10:10 AM
web/settings/account-settings.css
3 ↗(On Diff #10015)

It's a good question. My guess is that the intention here was to limit the width so the layout looks good. If for wider screens the component would be wider, then why not to make it wider from the beginning? Currently the ratio of this component to the enclosing component (my account page) is about 0.38 and to the whole screen it is 0.32 - neither of these looks like the intended proportions. Also, if the intention was to make this resizable, then a percentage would be used in Figma from the beginning.

So overall, I think we should keep it, but I'm not sure. @atul @benschac what are your thoughts / suggestions?

8 ↗(On Diff #10015)

I wanted to reduce the confusion by showing only the relevant part, but it looks like this introduced even more confusion. Going to be more mindful about this in the future.

web/settings/account-settings.css
3 ↗(On Diff #10015)

I've created a task where we can discuss this issue https://linear.app/comm/issue/ENG-852/consider-making-page-layout-responsive

This revision was automatically updated to reflect the committed changes.