Page MenuHomePhabricator

[native] Create a modal where invite links are displayed
ClosedPublic

Authored by tomek on May 5 2023, 8:51 AM.
Tags
None
Referenced Files
Unknown Object (File)
Sat, Nov 23, 9:11 PM
Unknown Object (File)
Wed, Nov 20, 4:53 PM
Unknown Object (File)
Tue, Nov 12, 11:22 AM
Unknown Object (File)
Fri, Nov 8, 1:47 AM
Unknown Object (File)
Thu, Nov 7, 4:59 PM
Unknown Object (File)
Tue, Nov 5, 8:59 AM
Unknown Object (File)
Mon, Nov 4, 4:14 PM
Unknown Object (File)
Mon, Nov 4, 4:14 PM

Details

Summary

In this modal we display a link and allow user to copy it.

Depends on D7728

Test Plan

Display a modal and click copy button - a pasteboard should contain the link.

Diff Detail

Repository
rCOMM Comm
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

Harbormaster returned this revision to the author for changes because remote builds failed.May 5 2023, 9:04 AM
Harbormaster failed remote builds in B19074: Diff 26122!

Use resolved thread name

tomek requested review of this revision.May 8 2023, 4:20 AM
native/navigation/view-invite-links-modal.react.js
1 ↗(On Diff #26172)

Nit: add newline after Flow declaration

native/themes/colors.js
98–100 ↗(On Diff #26172)

Can you provide pictures of the light mode please?

native/themes/colors.js
98–100 ↗(On Diff #26172)

That's a good question! But I have to fix the light mode first...

The design should be updated

Update the approach - use stack inside modal

native/themes/colors.js
138–139 ↗(On Diff #26547)

Do we really need a new family of colors for invite links? Did you try finding any existing families we could use? cc @ted

native/themes/colors.js
138–139 ↗(On Diff #26547)

For inviteLinkHeaderColor, the only other entry with the same color in dark theme is listSeparatorLabel - definitely not a good match.
For inviteLinkButtonBackground, there are a couple other options and there's one which matches both themes modalSeparator - but I don't think we should use it.

Do you have a suggestion of how we should approach this?

For me it seems like spending too much time on such a simple thing as colors isn't effective. We should probably start with creating consistent design, with a couple of predefined color pairs (light and dark) and then update all the components to use them.

native/themes/colors.js
138–139 ↗(On Diff #26547)

Do you have a suggestion of how we should approach this?

My general feeling is that invite link buttons and headers should not be specific to invite links. We should rely on a consistent design system for things like buttons and headers.

I'm pushing back here because this pattern of "every engineer introduces their own button for their screen" leads to an incredibly fractured design system.

Can you sync with @ted to figure out if invite link button / headers REALLY need to be distinct? If yes, can we come up with a more generic name for them here that describes what the button / header looks like, and how it can be reused?

Use redux instead of local state. The colors still need to be updated.

native/invite-links/view-invite-links-screen.react.js
61–63 ↗(On Diff #26676)
native/invite-links/view-invite-links-screen.react.js
61–63 ↗(On Diff #26676)

Looks good, but would love to see an opinion from someone more experienced in @react-navigation (cc. @inka)

This revision is now accepted and ready to land.May 23 2023, 5:23 AM
native/themes/colors.js
138–139 ↗(On Diff #26547)

Talked with @ted and he suggested that:

  1. inviteLinkLinkColor as a new color should stay
  2. inviteLinkButtonBackground - we could use instead panelSecondaryForeground. Which makes sense from colors point of view, but we should then find a new name as using foreground as a background is probably worse than introducing a new color.

As @ted is off for now, I don't think it makes sense to block this diff on this change.

Looks good

native/invite-links/view-invite-links-screen.react.js
61–63 ↗(On Diff #26676)

I can't find the code where this is changed based on the permissions

native/invite-links/view-invite-links-screen.react.js
61–63 ↗(On Diff #26676)

It's a lot later in the stack D7885

As @ted is off for now, I don't think it makes sense to block this diff on this change.

Can you create a DES task and assign it to Ted, to make sure we follow up after he is back?

As @ted is off for now, I don't think it makes sense to block this diff on this change.

Can you create a DES task and assign it to Ted, to make sure we follow up after he is back?

Sure, I've created https://linear.app/comm/issue/DES-90/try-to-use-existing-colors-on-view-invite-links-screen for it.