Page MenuHomePhabricator

[native] Introduce disable link button
ClosedPublic

Authored by tomek on May 22 2023, 6:32 AM.
Tags
None
Referenced Files
Unknown Object (File)
Fri, Dec 20, 1:28 AM
Unknown Object (File)
Thu, Dec 19, 2:32 PM
Unknown Object (File)
Sat, Dec 14, 7:20 AM
Unknown Object (File)
Sun, Dec 1, 1:21 PM
Unknown Object (File)
Fri, Nov 29, 8:55 AM
Unknown Object (File)
Nov 25 2024, 8:47 PM
Unknown Object (File)
Nov 25 2024, 6:25 PM
Unknown Object (File)
Nov 18 2024, 4:31 AM

Details

Summary

The button is visible only when a link is present.
Clicking the button results in alert being shown where a user is asked to confirm the action.
Confirming results in the link being disabled.

disable-button.png (2×1 px, 166 KB)

disable-button-confirm.png (2×1 px, 695 KB)

light-mode-disable.png (2×1 px, 171 KB)

Depends on D7907

Test Plan

Check if the button appears only when a link is present.
Check if clicking it results in an alert.
Check if canceling doesn't disable the link.
Check if confirming disables the link.

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

tomek requested review of this revision.May 22 2023, 6:50 AM
native/themes/colors.js
140 ↗(On Diff #26775)

Hey @tomek! I took a look at our color.js file, and did notice that errorDark50 isn't used in any other components yet. I know the designs are using errorDark50, but thinking about it, it may be better to use

vibrantRedButton: designSystemColors.errorPrimary

My only concern with the primary color was how bright it may be and attention grabbing, but we already have components using the primary in our system. What do you think?

native/themes/colors.js
140 ↗(On Diff #26775)

For me there's a value in distinguishing between error and destructive actions. So having different colors for them makes sense. But merging these also is an option.

I've checked how it looks like with an alternative color. It's indeed brighter. But compared to the older color, that button looks a little bit like it's disabled. (but only in comparison)

disable-button-bright.png (2×1 px, 168 KB)

native/themes/colors.js
140 ↗(On Diff #26775)

@ted is on vacation now, but I agree the darker color looks a little bit disabled in comparison. Personally, I think I'd prefer the brighter red

kamil added inline comments.
native/invite-links/manage-public-link-screen.react.js
125–131 ↗(On Diff #26886)

Wondering how this button looks in light mode

This revision is now accepted and ready to land.May 25 2023, 4:33 AM
bartek added inline comments.
native/invite-links/manage-public-link-screen.react.js
103–105 ↗(On Diff #26886)

Does this message look well in native alert box? It's pretty long

tomek added inline comments.
native/invite-links/manage-public-link-screen.react.js
103–105 ↗(On Diff #26886)

In the summary there's a screenshot with it - looks ok, I guess.

125–131 ↗(On Diff #26886)

Included a screenshot in the summary

This revision was automatically updated to reflect the committed changes.