[web] Introduce empty account page
ClosedPublic
Actions

Authored by tomek on Mar 2 2022, 6:14 AM.

Details

Reviewers

Commits

rCOMM7c1fe3eafb4c: [web] Introduce empty account page

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

Test Plan

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

Repository

rCOMM Comm

Lint

No Lint Coverage

Unit

No Test Coverage

tomek requested review of this revision.Mar 2 2022, 6:22 AM

• benschac requested changes to this revision.Mar 2 2022, 8:15 AM

• benschac added inline comments.

web/settings/account-settings.css
2	The padding in figma is saying 24px. why 40px?
8	Looking at figma, I don't think this should be semi bold. 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	I think you might be looking at the wrong "My Account" in Figma.. I think it's this one:

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

web/settings/account-settings.css
2	The padding here is correct: https://www.figma.com/file/a1nkbWgbgjRlrOY9LVurTz/Comm-%2F-Desktop-app?node-id=1272%3A88751
3	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	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	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	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.