Page MenuHomePhabricator

[web] Introduce disable link modal
ClosedPublic

Authored by tomek on Jul 3 2023, 6:39 AM.
Tags
None
Referenced Files
F3388047: D8419.diff
Fri, Nov 29, 12:27 PM
Unknown Object (File)
Tue, Nov 26, 5:31 AM
Unknown Object (File)
Tue, Nov 26, 4:46 AM
Unknown Object (File)
Sat, Nov 23, 12:47 PM
Unknown Object (File)
Tue, Nov 19, 3:28 PM
Unknown Object (File)
Mon, Nov 11, 4:13 AM
Unknown Object (File)
Sun, Nov 10, 12:32 PM
Unknown Object (File)
Thu, Nov 7, 10:04 AM
Subscribers

Details

Summary

When a link is created, on manage links modal there should be a button which opens disable link modal.
https://www.figma.com/file/a1nkbWgbgjRlrOY9LVurTz/Comm-%2F-Desktop-app?node-id=11341%3A280056&mode=dev

manage-and-disable.png (1×1 px, 122 KB)

Disable links modal should allow disabling link or going back.
https://www.figma.com/file/a1nkbWgbgjRlrOY9LVurTz/Comm-%2F-Desktop-app?node-id=11341%3A280530&mode=dev

disable.png (744×1 px, 82 KB)

Depends on D8417

Test Plan

Check if disable link button is present only when there's a link.
Click a button and check if it is possible to go back.
Click disable link button and check if it disables a link.

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

tomek added a reviewer: ashoat.
Harbormaster returned this revision to the author for changes because remote builds failed.Jul 3 2023, 6:52 AM
Harbormaster failed remote builds in B20688: Diff 28350!
tomek requested review of this revision.Jul 3 2023, 7:30 AM
ashoat requested changes to this revision.Jul 3 2023, 11:56 AM
ashoat added inline comments.
web/components/button.react.js
10–12 ↗(On Diff #28351)

I didn't know about this syntax! Good to know

However, I'm not a fan of the inconsistency between backgroundColor and border-color. Can you either make it consistent, or otherwise explain why it can't be?

web/invite-links/manage/disable-link-modal.react.js
36–41 ↗(On Diff #28351)

I shared feedback about this copy in ENG-4304 : Update invite links copy under point 9. Can you update it to reflect my feedback?

web/invite-links/manage/edit-link-modal.react.js
52 ↗(On Diff #28351)

I'm a little confused why I don't see this copy anywhere in native. Can you explain?

This revision now requires changes to proceed.Jul 3 2023, 11:56 AM
web/components/button.react.js
10–12 ↗(On Diff #28351)

Quickly reading Button code gave me an impression that backgroundColor has a special meaning there. I wanted to customize border-color and figured it is used as a style prop so I thought that using css prop names is the correct solution. But after making a deeper investigation I realized I was mistaken.

So according to the docs https://legacy.reactjs.org/docs/dom-elements.html#style we should use camel case for the prop names. The fact that border-color worked correctly is surprising - it shouldn't work. So I'm going to update this diff with camel case name.

By the way, I realized our Button component is really inconvenient to use. We're mixing two concepts: its shape and color, e.g. when using outline variant we're forcing var(--btn-bg-outline) color, unless we're overriding it like in this diff. For this component to be convenient to use, I would expect to be able to specify variant="outline" buttonColor="danger" which would result in red, outlined button. I've created a task for that https://linear.app/comm/issue/ENG-4308/allow-web-button-color-and-variant-to-be-specified-independently.

Update copy and button props

web/invite-links/manage/disable-link-modal.react.js
36–41 ↗(On Diff #28351)

web-disable-link.png (660×1 px, 57 KB)

web/invite-links/manage/edit-link-modal.react.js
52 ↗(On Diff #28351)
ashoat requested changes to this revision.Jul 5 2023, 10:30 AM
ashoat added a subscriber: ted.
ashoat added inline comments.
web/invite-links/manage/disable-link-modal.react.js
37–38 ↗(On Diff #28410)

It's considered bad practice in HTML to use <br /> for whitespace. Instead, you should probably implement this as two <p> tags. Margin and padding should be used for whitespace, and whitespace characters and <br /> should be avoided

web/invite-links/manage/edit-link-modal.react.js
52 ↗(On Diff #28351)
  1. I'm not sure what you mean by "doesn't have a related description"
  2. Thanks for linking the designs! That said, I'm still not clear on why the button copy should be different. Can you sync with @ted and check if there is a good explanation? If not, we should aim for consistency. Please make sure to talk about the disable button / section looking different, as well as the "Save public link" button having different copy
This revision now requires changes to proceed.Jul 5 2023, 10:30 AM
web/invite-links/manage/disable-link-modal.react.js
37–38 ↗(On Diff #28410)

Overall I agree that there are some downsides of using <br /> or \n, especially when they are used multiple times just to introduce a margin between text blocks. But in this case it seems like the intention is different: in the design this whole copy is present as a single block of text. Additionally the height of the space between the parts looks like one line height. I don't think it is a good idea to try to recompute line height and somehow assign the value to a margin.

I think that using <br /> here is better in matching the design. But a separate question is if we really need a margin that is as high as one line height. And the answer is that probably not (@ted do you agree?). I'll update the code to use margin (or gap) instead (also in other places where this kind of separation was used).

https://www.figma.com/file/a1nkbWgbgjRlrOY9LVurTz/Comm-%2F-Desktop-app?node-id=11341%3A280517&mode=dev

web/invite-links/manage/edit-link-modal.react.js
52 ↗(On Diff #28351)
  1. By the description I was talking about this You may also disable the community public link text. In native designs it isn't present.
  2. Sure. I've sent a message to @ted and we will discuss the issue.
web/invite-links/manage/edit-link-modal.react.js
52 ↗(On Diff #28351)

Got your message, thank you Tomek

web/invite-links/manage/edit-link-modal.react.js
52 ↗(On Diff #28351)

The result of the discussion:

  1. Save & enable public link button text should be used consistently on web and native
  2. Disable link button should contain just Disable text
  3. Both platforms should contain You may also disable the community public link text. On native it will be displayed above Disable button. The button and the text will be placed as one section.

new-navite-disable.png (1×816 px, 106 KB)

web/invite-links/manage/disable-link-modal.react.js
37–38 ↗(On Diff #28410)

Agree we don't need it to be that high. Let's avoid <br /> and \n for spacing, and make the spacing between paragraphs less than one full line height

web/invite-links/manage/disable-link-modal.react.js
37–38 ↗(On Diff #28410)

I've introduced 12px gap

web-disable-link-gap.png (656×1 px, 62 KB)

web/invite-links/manage/edit-link-modal.react.js
52 ↗(On Diff #28351)

Native updates will be made in a separate diff.

ashoat added inline comments.
web/invite-links/manage/disable-link-modal.react.js
37–38 ↗(On Diff #28410)

The image doesn't seem to have attached

This revision is now accepted and ready to land.Jul 7 2023, 11:21 AM
web/invite-links/manage/edit-link-modal.react.js
52 ↗(On Diff #28351)

Created D8444 with these

web/invite-links/manage/disable-link-modal.react.js
37–38 ↗(On Diff #28410)

Sorry about that! Let me check if simple drag-and-drop works correctly (it should according to https://www.notion.so/commapp/Attaching-files-and-images-on-Phabricator-af8e2195fc91462689cc8de5c96a6759)
{F630056}

web/invite-links/manage/disable-link-modal.react.js
37–38 ↗(On Diff #28410)

It seems like drag-n-drop doesn't work any more