Page MenuHomePhabricator

[native] linked devices component
ClosedPublic

Authored by varun on Jul 28 2024, 8:13 PM.
Tags
None
Referenced Files
F3098583: D12907.id43579.diff
Tue, Oct 29, 6:42 PM
Unknown Object (File)
Mon, Oct 28, 8:16 PM
Unknown Object (File)
Sun, Oct 27, 9:21 AM
Unknown Object (File)
Sun, Oct 27, 2:26 AM
Unknown Object (File)
Fri, Oct 25, 9:35 PM
Unknown Object (File)
Wed, Oct 23, 4:26 PM
Unknown Object (File)
Tue, Oct 22, 10:44 AM
Unknown Object (File)
Tue, Oct 22, 10:19 AM
Subscribers

Details

Summary

Depends on D12884

UI for linked devices. You can navigate from this screen to the SecondaryDeviceQRCodeScanner using the Add button on the top right

next diff will add a bottom drawer for interacting with each device

Test Plan

successfully added a new device, it appeared in the list. navigation works as expected

Simulator Screenshot - iPhone 15 Pro Max - 2024-07-29 at 00.08.07.png (2×1 px, 153 KB)

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

varun requested review of this revision.Jul 28 2024, 8:30 PM

Neat! 😄 I'm not a React/UI expert so resigning, but I'm glad my gist helped.

Ideas for the future:

  • In the future, we can add a possibility of giving them user-defined friendly names (not sure where they could be stored and how to sync them)
native/profile/linked-devices-list-item.react.js
48–54 ↗(On Diff #42857)

We could potentially retrieve thisDeviceID and also add a label that this is the current device.

We should probably fetch it once in the parent list component and pass via props, but I am not sure

Looks ok to me, but we need to make sure @ashoat will take a look before landing

native/profile/linked-devices-list-item.react.js
6–7 ↗(On Diff #42857)

Can be merged

28–42 ↗(On Diff #42857)

We rarely use switch in our code. Can you replace it with if-else?

51 ↗(On Diff #42857)

After appending primary it is no longer a base label - doesn't matter that much, but maybe we can make the naming less confusing.

58 ↗(On Diff #42857)

Why do we use a TouchableOpacity without onPress? If the component isn't pressable, we shouldn't use a component that handles pressing.

This revision is now accepted and ready to land.Jul 29 2024, 12:41 AM

Adding myself as blocking based on @tomek's comment above

This revision now requires review to proceed.Jul 31 2024, 8:58 PM
ashoat added inline comments.
native/profile/keyserver-selection-list.react.js
90–96 ↗(On Diff #42857)

Nice find!

native/profile/linked-devices-list-item.react.js
48–54 ↗(On Diff #42857)

I think that's a good idea. I might prefer to use (this device) to make it more consistent with (primary). We should also consider how to format a device when it is both the current device, and the primary device

51 ↗(On Diff #42857)

You can avoid having to name the final result:

const baseLabel = deviceID.substr(0, 7);
if (!isPrimary) {
  return baseLabel;
}
return `${baseLabel} (primary)`;

Could potentially also combine the last four lines into a single-line ternary

58 ↗(On Diff #42857)

Yeah this doesn't make sense to me either

This revision is now accepted and ready to land.Aug 2 2024, 2:34 PM
native/profile/linked-devices-list-item.react.js
58 ↗(On Diff #42857)

next diff adds an onPress

This revision was automatically updated to reflect the committed changes.

We should also consider how to format a device when it is both the current device, and the primary device

Hmm, it seems like not much thought was put here... you just put both parentheticals in at the same time.