Page MenuHomePhabricator

[web] Add logout button
ClosedPublic

Authored by tomek on Mar 2 2022, 8:04 AM.
Tags
None
Referenced Files
F3295668: D3322.diff
Sat, Nov 16, 10:37 PM
Unknown Object (File)
Wed, Nov 13, 12:35 PM
Unknown Object (File)
Wed, Nov 13, 3:57 AM
Unknown Object (File)
Tue, Nov 5, 10:11 PM
Unknown Object (File)
Oct 14 2024, 2:42 AM
Unknown Object (File)
Oct 14 2024, 2:42 AM
Unknown Object (File)
Oct 14 2024, 2:42 AM
Unknown Object (File)
Oct 14 2024, 2:42 AM

Details

Summary

Add logout button and make sure that layout remains correct even if username is long
https://www.figma.com/file/a1nkbWgbgjRlrOY9LVurTz/Comm-%2F-Desktop-app?node-id=1272%3A88750

account-settings-6.png (710×1 px, 36 KB)

account-settings-7.png (770×1 px, 40 KB)

Depends on D3321

Test Plan

Check if the page renders correctly.
Modify username using dev tools and make it longer - check if it is rendered correctly.
Click logout button - user should be logged out.

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

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

Could we use button component: https://github.com/CommE2E/comm/blob/master/web/components/button.react.js.

The css is very similar: https://github.com/CommE2E/comm/blob/master/web/components/button.css

It's only missing the white-space: nowrap;

This revision now requires changes to proceed.Mar 2 2022, 8:43 AM
tomek requested review of this revision.Mar 4 2022, 7:13 AM
tomek added inline comments.
web/settings/account-settings.css
45 ↗(On Diff #10028)

Could you explain a bit more about how to use this component? It doesn't look reusable... it sets a different color and background and at the same time doesn't have nowrap. The solution where a couple of properties are overwritten by client code is fragile, because if the component's code change it might break the clients. I also don't think that just using the class from the component makes sense, as it is also fragile.
The correct solution here would be to either provide a reusable component that takes a couple of props like color and background-color, or introduce a new variant, e.g. text. @benschac what are your thoughts here?

web/settings/account-settings.css
45 ↗(On Diff #10028)

This is how it's used in user-settings-modal The thought is to add a new variant for each button. But, maybe having an approach where the button takes in color, background, size and shape props would be more flexible.

<Button
          variant="danger"
          type="submit"
          onClick={this.onDelete}
          disabled={inputDisabled}
        >
          Delete account
        </Button>
web/settings/account-settings.css
45 ↗(On Diff #10028)

Having a really reusable button might make sense, but I'm not sure if this is the best idea. Even if we have some reasonable defaults, it will be harder to use than a limited set of predefined variants (especially when dealing with different themes).
Ultimately, the design should define a couple of variants and no other variant should appear in the app. In that case, introducing a text variant should be the best idea here, but I'll need to check the designs if that's possible.
As a side note: a configurable button (or any component) should be able to receive e.g. color props, which isn't that comfortable without any CSSinJS solution.

web/settings/account-settings.css
45 ↗(On Diff #10028)

I think it doesn't make sense to block this diff on this comment. I created a linear issue https://linear.app/comm/issue/ENG-843/introduce-text-button-type where a new button type will be introduced and used here. @benschac

This revision is now accepted and ready to land.Mar 8 2022, 8:43 AM
This revision was automatically updated to reflect the committed changes.
web/settings/account-settings.react.js
16

We should pretty much never call a server call "nakedly" like this. It should be wrapped in a dispactionActionPromise. This resulted in ENG-1604