Page MenuHomePhabricator
Differential D7730

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

Authored by tomek on May 5 2023, 8:51 AM.
Tags
None
Referenced Files
Unknown Object (File)
Tue, Apr 1, 3:38 PM
Unknown Object (File)
Tue, Apr 1, 8:43 AM
Unknown Object (File)
Sun, Mar 30, 9:11 PM
Unknown Object (File)
Sun, Mar 30, 9:04 PM
Unknown Object (File)
Mon, Mar 24, 2:10 PM
Unknown Object (File)
Mon, Mar 17, 1:26 PM
Unknown Object (File)
Mon, Mar 17, 1:26 PM
Unknown Object (File)
Mon, Mar 17, 1:26 PM
View All Files
Subscribers
ashoat
atul
• ted

Details

Reviewers
bartek
kamil
inka
Commits
rCOMMd33be322bf98: [native] Create a modal where invite links are displayed
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

tomek created this revision.May 5 2023, 8:51 AM
Herald added subscribers: atul, ashoat. · View Herald TranscriptMay 5 2023, 8:51 AM
tomek added a child revision: D7731: [native] Display a button and navigate to invite links if they are present.May 5 2023, 8:54 AM
tomek added a child revision: D7733: [native] Display a button and navigate to invite links if they are present.May 5 2023, 9:00 AM
tomek edited the summary of this revision. (Show Details)May 5 2023, 9:02 AM
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!
tomek updated this revision to Diff 26172.EditedMay 8 2023, 4:02 AM

Use resolved thread name

Harbormaster completed remote builds in B19118: Diff 26172.May 8 2023, 4:20 AM
tomek requested review of this revision.May 8 2023, 4:20 AM
ashoat added inline comments.May 8 2023, 6:48 AM
native/navigation/view-invite-links-modal.react.js
1 ↗(On Diff #26172)

Nit: add newline after Flow declaration

inka added inline comments.May 9 2023, 3:31 AM
native/themes/colors.js
98–100 ↗(On Diff #26172)

Can you provide pictures of the light mode please?

tomek added inline comments.May 10 2023, 6:42 AM
native/themes/colors.js
98–100 ↗(On Diff #26172)

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

tomek updated this revision to Diff 26348.May 10 2023, 7:16 AM

Fix the light mode

tomek edited the summary of this revision. (Show Details)May 10 2023, 7:19 AM
tomek updated this revision to Diff 26349.May 10 2023, 7:21 AM

Add newline

Harbormaster failed remote builds in B19246: Diff 26348!May 10 2023, 7:30 AM
Harbormaster failed remote builds in B19247: Diff 26349!May 10 2023, 7:34 AM
tomek updated this revision to Diff 26440.May 15 2023, 3:10 AM

Fix eslint

Harbormaster completed remote builds in B19326: Diff 26440.May 15 2023, 3:28 AM
tomek updated this revision to Diff 26515.May 16 2023, 4:10 AM

Rebase

Herald added a subscriber: • ted. · View Herald TranscriptMay 16 2023, 4:10 AM
Harbormaster completed remote builds in B19380: Diff 26515.May 16 2023, 4:27 AM
tomek planned changes to this revision.May 16 2023, 5:52 AM

The design should be updated

tomek updated this revision to Diff 26547.May 16 2023, 8:32 AM

Update the approach - use stack inside modal

tomek edited the summary of this revision. (Show Details)May 16 2023, 8:39 AM
Harbormaster completed remote builds in B19404: Diff 26547.May 16 2023, 8:54 AM
ashoat added inline comments.May 16 2023, 9:48 AM
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

tomek added inline comments.May 17 2023, 1:46 AM
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.

ashoat added inline comments.May 17 2023, 6:21 AM
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?

tomek updated this revision to Diff 26676.May 19 2023, 9:47 AM

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

tomek edited parent revisions, added: D7877: [lib] Create a store in redux for the invite links; removed: D7728: [lib] Add client side boilerplate for the new endpoint.May 19 2023, 9:47 AM
Harbormaster completed remote builds in B19511: Diff 26676.May 19 2023, 10:05 AM
inka added inline comments.May 23 2023, 4:42 AM
native/invite-links/view-invite-links-screen.react.js
61–63 ↗(On Diff #26676)

What designs are you using? I see a different message https://www.figma.com/file/a1nkbWgbgjRlrOY9LVurTz/Comm-%2F-Desktop-app?type=design&node-id=12485-235674&t=5pjj80bZzIrM3YOD-4

tomek added inline comments.May 23 2023, 5:19 AM
native/invite-links/view-invite-links-screen.react.js
61–63 ↗(On Diff #26676)

The state depends on the role of a user. This screen is for regular users https://www.figma.com/file/a1nkbWgbgjRlrOY9LVurTz/Comm-%2F-Desktop-app?type=design&node-id=12485-239352&t=mSlYMlGynVnD8xDZ-0

kamil accepted this revision.May 23 2023, 5:23 AM

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
tomek updated this revision to Diff 26872.May 23 2023, 5:26 AM
tomek added a parent revision: D7728: [lib] Add client side boilerplate for the new endpoint.

Update color name

tomek added inline comments.May 23 2023, 5:30 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.

Harbormaster completed remote builds in B19664: Diff 26872.May 23 2023, 5:45 AM
inka added a comment.May 23 2023, 6:53 AM

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

inka accepted this revision.May 23 2023, 6:54 AM
tomek added inline comments.May 23 2023, 7:20 AM
native/invite-links/view-invite-links-screen.react.js
61–63 ↗(On Diff #26676)

It's a lot later in the stack D7885

ashoat added a comment.May 23 2023, 11:26 AM

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?

tomek added a comment.May 24 2023, 12:57 AM
In D7730#235617, @ashoat wrote:

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.

Closed by commit rCOMMd33be322bf98: [native] Create a modal where invite links are displayed (authored by tomek). · Explain WhyMay 24 2023, 4:23 AM
This revision was automatically updated to reflect the committed changes.
tomek added a commit: rCOMMd33be322bf98: [native] Create a modal where invite links are displayed.

Revision Contents
Changeset List

PathSize
native/
navigation/
7 lines
3 lines
140 lines
themes/
4 lines

Diff 26440

View Options

native/navigation/root-navigator.react.js

Show First 20 Lines • Show All 130 Lines • ▼ Show 20 Lines
const styles = StyleSheet.create({ const styles = StyleSheet.create({
dummyMessage: { dummyMessage: {
paddingHorizontal: 12, paddingHorizontal: 12,
paddingVertical: 6, paddingVertical: 6,
}, },
message: { message: {
overflow: 'hidden', overflow: 'hidden',
paddingHorizontal: 12, paddingHorizontal: 12,
paddingVertical: 6, paddingVertical: 8,
}, },
text: { text: {
fontFamily: 'Arial', fontFamily: 'Arial',
fontSize: 18, fontSize: 14,
benschacAuthorUnsubmitted
Done
Inline Actions

I actually think this is a bit too small. But that's what's in the design.

https://www.figma.com/file/L675ETKDnGaSwlpZAw4MIC/Mobile-App?node-id=351%3A455

@ashoat - I think 16px looks better.

image.png (1×2 px, 2 MB)

benschac: I actually think this is a bit too small. But that's what's in the design. https://www.figma.
lineHeight: 22,
}, },
}); });
export { InnerTextMessage, dummyNodeForTextMessageHeightMeasurement }; export { InnerTextMessage, dummyNodeForTextMessageHeightMeasurement };
View Options

native/navigation/route-names.js

Show First 20 Lines • Show All 130 Lines • ▼ Show 20 Lines
const styles = StyleSheet.create({ const styles = StyleSheet.create({
dummyMessage: { dummyMessage: {
paddingHorizontal: 12, paddingHorizontal: 12,
paddingVertical: 6, paddingVertical: 6,
}, },
message: { message: {
overflow: 'hidden', overflow: 'hidden',
paddingHorizontal: 12, paddingHorizontal: 12,
paddingVertical: 6, paddingVertical: 8,
}, },
text: { text: {
fontFamily: 'Arial', fontFamily: 'Arial',
fontSize: 18, fontSize: 14,
benschacAuthorUnsubmitted
Done
Inline Actions

I actually think this is a bit too small. But that's what's in the design.

https://www.figma.com/file/L675ETKDnGaSwlpZAw4MIC/Mobile-App?node-id=351%3A455

@ashoat - I think 16px looks better.

image.png (1×2 px, 2 MB)

benschac: I actually think this is a bit too small. But that's what's in the design. https://www.figma.
lineHeight: 22,
}, },
}); });
export { InnerTextMessage, dummyNodeForTextMessageHeightMeasurement }; export { InnerTextMessage, dummyNodeForTextMessageHeightMeasurement };
View Options

native/navigation/view-invite-links-modal.react.js

Show First 20 Lines • Show All 130 Lines • ▼ Show 20 Lines
const styles = StyleSheet.create({ const styles = StyleSheet.create({
dummyMessage: { dummyMessage: {
paddingHorizontal: 12, paddingHorizontal: 12,
paddingVertical: 6, paddingVertical: 6,
}, },
message: { message: {
overflow: 'hidden', overflow: 'hidden',
paddingHorizontal: 12, paddingHorizontal: 12,
paddingVertical: 6, paddingVertical: 8,
}, },
text: { text: {
fontFamily: 'Arial', fontFamily: 'Arial',
fontSize: 18, fontSize: 14,
benschacAuthorUnsubmitted
Done
Inline Actions

I actually think this is a bit too small. But that's what's in the design.

https://www.figma.com/file/L675ETKDnGaSwlpZAw4MIC/Mobile-App?node-id=351%3A455

@ashoat - I think 16px looks better.

image.png (1×2 px, 2 MB)

benschac: I actually think this is a bit too small. But that's what's in the design. https://www.figma.
lineHeight: 22,
}, },
}); });
export { InnerTextMessage, dummyNodeForTextMessageHeightMeasurement }; export { InnerTextMessage, dummyNodeForTextMessageHeightMeasurement };
View Options

native/themes/colors.js

Show First 20 Lines • Show All 130 Lines • ▼ Show 20 Lines
const styles = StyleSheet.create({ const styles = StyleSheet.create({
dummyMessage: { dummyMessage: {
paddingHorizontal: 12, paddingHorizontal: 12,
paddingVertical: 6, paddingVertical: 6,
}, },
message: { message: {
overflow: 'hidden', overflow: 'hidden',
paddingHorizontal: 12, paddingHorizontal: 12,
paddingVertical: 6, paddingVertical: 8,
}, },
text: { text: {
fontFamily: 'Arial', fontFamily: 'Arial',
fontSize: 18, fontSize: 14,
benschacAuthorUnsubmitted
Done
Inline Actions

I actually think this is a bit too small. But that's what's in the design.

https://www.figma.com/file/L675ETKDnGaSwlpZAw4MIC/Mobile-App?node-id=351%3A455

@ashoat - I think 16px looks better.

image.png (1×2 px, 2 MB)

benschac: I actually think this is a bit too small. But that's what's in the design. https://www.figma.
lineHeight: 22,
}, },
}); });
export { InnerTextMessage, dummyNodeForTextMessageHeightMeasurement }; export { InnerTextMessage, dummyNodeForTextMessageHeightMeasurement };