Page MenuHomePhabricator

[web] Create manage links modal
ClosedPublic

Authored by tomek on Jul 3 2023, 5:03 AM.
Tags
None
Referenced Files
F3388783: D8413.diff
Fri, Nov 29, 3:58 PM
Unknown Object (File)
Sat, Nov 23, 2:29 PM
Unknown Object (File)
Sun, Nov 17, 2:05 PM
Unknown Object (File)
Sun, Nov 10, 12:32 PM
Unknown Object (File)
Sat, Nov 9, 4:04 AM
Unknown Object (File)
Fri, Nov 1, 8:20 AM
Unknown Object (File)
Fri, Nov 1, 8:20 AM
Unknown Object (File)
Fri, Nov 1, 8:20 AM
Subscribers

Details

Summary
Test Plan

Click manage links button and check if the modal is correct, both when a public link is present and when a community doesn't have a link.

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

tomek added a reviewer: ashoat.
tomek requested review of this revision.Jul 3 2023, 5:22 AM

Accepting to unblock, but have a couple comments. The title stuff could potentially be handled in a follow-up if that's your preference, as I suspect it might require design changes

web/invite-links/manage-invite-links-modal.react.js
43 ↗(On Diff #28343)

It looks like we're using spaces for padding here

64 ↗(On Diff #28343)

In ENG-4304, I mention in a couple places that I'd prefer to avoid putting user-generated names in screen / modal titles.

Is there an alternate design we could take where we show the community name inside the content rather than the title?

Separately, it appears that the equivalent on native (ManagePublicLinkScreen) uses a different title ("Public Link"). Let me know if I'm looking at the wrong component, but if it's right – should we aim for consistency?

This revision is now accepted and ready to land.Jul 3 2023, 10:53 AM
tomek requested review of this revision.Jul 5 2023, 3:19 AM
tomek added inline comments.
web/invite-links/manage-invite-links-modal.react.js
43 ↗(On Diff #28343)

That's correct. In the design https://www.figma.com/file/a1nkbWgbgjRlrOY9LVurTz/Comm-%2F-Desktop-app?node-id=11341%3A283370&mode=dev it looks like the intention is for this link to look like a part of a text block. To achieve that we should have a gap that is exactly as long as a width of a space char - and the easiest way to have that is by rendering the space. I can set e.g. a fixed margin instead, but it won't be guaranteed that its width will always be equal to a single space width.

What risks can you see in the current approach?

64 ↗(On Diff #28343)

In the future, this screen will also contain custom links https://www.figma.com/file/a1nkbWgbgjRlrOY9LVurTz/Comm-%2F-Desktop-app?node-id=11341%3A277530&mode=dev. Also, Public link is already a name of a section within this modal. We can consider using e.g. Manage invite links as a modal title, which makes a lot of sense because the button which opens the modal has this label.

As for native, there's no 1:1 matching within these. The screen that uses Public Link name on native corresponds to a modal this is open after clicking Edit public link / Enable buttons here.

web/invite-links/manage-invite-links-modal.react.js
43 ↗(On Diff #28343)

On native, we're displaying this button in a new line

native-invite-new-line.png (184×409 px, 13 KB)

web/invite-links/manage-invite-links-modal.react.js
43 ↗(On Diff #28343)

Ah, I didn't realize from the code that <Button variant="text" /> displays a text link. This makes sense now

64 ↗(On Diff #28343)

Let's go with "Manage invite links"!

This revision is now accepted and ready to land.Jul 5 2023, 10:16 AM

Update modal name

web/invite-links/manage-invite-links-modal.react.js
64 ↗(On Diff #28343)

manage-new-title.png (496×1 px, 50 KB)

web/invite-links/manage-invite-links-modal.react.js
64 ↗(On Diff #28343)

Heads-up, this attachment is not showing up

This revision was automatically updated to reflect the committed changes.