Page MenuHomePhabricator

[web] Add password section
ClosedPublic

Authored by tomek on Mar 2 2022, 9:06 AM.
Tags
None
Referenced Files
F3297060: D3323.id10190.diff
Sun, Nov 17, 2:37 AM
F3296192: D3323.id10198.diff
Sun, Nov 17, 1:32 AM
Unknown Object (File)
Fri, Nov 15, 8:59 AM
Unknown Object (File)
Tue, Nov 5, 10:11 PM
Unknown Object (File)
Sun, Oct 20, 8:29 PM
Unknown Object (File)
Sun, Oct 20, 8:29 PM
Unknown Object (File)
Sun, Oct 20, 8:29 PM
Unknown Object (File)
Oct 16 2024, 4:28 PM

Details

Summary

Display a password section with a button that allows editing it. The editing will be implemented in one of the future diffs.
https://www.figma.com/file/a1nkbWgbgjRlrOY9LVurTz/Comm-%2F-Desktop-app?node-id=1272%3A88750

account-settings-9.png (878×1 px, 49 KB)

Depends on D3322

Test Plan

Display a page and check if it is rendered 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, 9:11 AM
web/settings/account-settings.react.js
44 ↗(On Diff #10029)

could we use the button component here?

This revision is now accepted and ready to land.Mar 3 2022, 12:36 PM

My personal preference would be for text to be left-aligned with the header... but what you did matches the Figma designs

0137-1.png (1×984 px, 207 KB)

In D3323#89766, @atul wrote:

My personal preference would be for text to be left-aligned with the header... but what you did matches the Figma designs

0137-1.png (1×984 px, 207 KB)

I think the reason for this design was that we would like to have horizontal lines between the items. It would be strange if the text started at the same point as the line. It would be also strange if the lines started earlier than the heading. So my guess is that it would be hard to find a design where we have the separators and the text is aligned with the header.

web/settings/account-settings.react.js
44 ↗(On Diff #10029)

None of the existing variants work ok here, but after checking our codebase it looks like we should instead use a - we're using it in a couple of similar contexts.

This revision was automatically updated to reflect the committed changes.