Page MenuHomePhabricator

[native] add ability to remove devices from linked devices bottom sheet
ClosedPublic

Authored by bartek on Aug 20 2024, 10:11 AM.
Tags
None
Referenced Files
Unknown Object (File)
Sat, Nov 9, 10:42 AM
Unknown Object (File)
Fri, Nov 8, 7:03 PM
Unknown Object (File)
Fri, Nov 8, 7:03 PM
Unknown Object (File)
Fri, Nov 8, 7:03 PM
Unknown Object (File)
Fri, Nov 8, 7:01 PM
Unknown Object (File)
Fri, Nov 8, 1:33 AM
F3148979: Screenshot 2024-11-04 at 3.51.44 PM.png
Mon, Nov 4, 1:41 PM
F3148975: Screenshot 2024-11-04 at 3.50.48 PM.png
Mon, Nov 4, 1:41 PM
Subscribers

Details

Summary

Depends on D13038

Test Plan

successfully removed secondary devices

Video of this, along with D13262 and D13747

Diff Detail

Repository
rCOMM Comm
Branch
hackathon (branched from master)
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

varun held this revision as a draft.
varun published this revision for review.Sep 2 2024, 7:19 PM
varun added reviewers: ashoat, tomek, bartek.
varun planned changes to this revision.Sep 2 2024, 7:22 PM
varun edited the summary of this revision. (Show Details)
varun edited the test plan for this revision. (Show Details)
varun edited the summary of this revision. (Show Details)

clean up

bartek requested changes to this revision.Sep 9 2024, 12:35 AM

Besides that one thing, device removal logic looks good

native/profile/linked-devices-bottom-sheet.react.js
94 ↗(On Diff #43944)
This revision now requires changes to proceed.Sep 9 2024, 12:35 AM
bartek edited reviewers, added: varun; removed: bartek.
This revision now requires review to proceed.Thu, Oct 17, 3:16 AM

Broadcast device list update to all peers

ashoat requested changes to this revision.Thu, Oct 17, 8:58 AM

I'm curious to see what this looks like. Given this is primarily a UI diff, can you share a screen capture video of the flow? (Easy to record with the built-in QuickTime Player app on macOS)

Requesting changes primarily so I can see what it looks before accepting. Sorry for the churn

native/profile/linked-devices-bottom-sheet.react.js
89–96 ↗(On Diff #45266)

Can these be done in parallel, or do we need to wait on the first before starting the second?

109 ↗(On Diff #45266)

We shouldn't capitalize the second word like this

This revision now requires changes to proceed.Thu, Oct 17, 8:58 AM
bartek edited the test plan for this revision. (Show Details)
bartek edited the test plan for this revision. (Show Details)
bartek edited the test plan for this revision. (Show Details)

Feedback: Fixed capitalizing, parallelized sending messages. Attached video to the test plan

ashoat requested changes to this revision.Fri, Oct 18, 6:29 AM

Video isn't attached – see here

This revision now requires changes to proceed.Fri, Oct 18, 6:29 AM
bartek added inline comments.
native/profile/linked-devices-bottom-sheet.react.js
89–96 ↗(On Diff #45266)

Good question. At first I was certain that it needs to be done sequentially, but realized it doesn't matter, the device list is already updated and these both are just sending Tunnelbroker messages. It doesn't matter who receives it first (peers or the logged out device).
Tested it with both a peer and the removed device being online, and it works

ashoat requested changes to this revision.Mon, Oct 21, 8:59 AM

Thanks for sharing the video! Unfortunately, the button appears over the home pill. Can you make sure that the button appears above the home pill? As an example, take a look at the use of useSafeAreaInsets in KeyserverSelectionBottomSheet

This revision now requires changes to proceed.Mon, Oct 21, 8:59 AM

Thanks for sharing the video! Unfortunately, the button appears over the home pill. Can you make sure that the button appears above the home pill? As an example, take a look at the use of useSafeAreaInsets in KeyserverSelectionBottomSheet

The useSafeAreaInsets is already used here. The issue turned out to be missing padding. The Keyserver bottom sheet is more complex and it was calculated differently there.

Simulator Screenshot - iPhone 15 Pro - 2024-10-25 at 10.25.13.png (2×1 px, 155 KB)

Add missing bottom padding

ashoat requested changes to this revision.Mon, Nov 4, 10:06 AM

This solution is ringing alarm bells. It feels like a hack... seems like you just added some padding to push the content up, but it's unclear to me why this padding needs to be added (what is it making up for?)

Unfortunately I won't be able to accept the diff without a deeper explanation. I feel compelled to have to check out your code myself to investigate, which is not a good pattern for a reviewer, and is made extra complicated here because you didn't rebase the final diff in the stack after this update...

This revision now requires changes to proceed.Mon, Nov 4, 10:06 AM

I looked at this more closely and indeed it appears that the prior revision is a hack.

The core issue here is a problem in native/bottom-sheet/bottom-sheet.react.js. Here is what I learned:

  • First, note that we override handleComponent. To see what part that renders, I used this patch to make it appear purple:
    Screenshot 2024-11-04 at 3.50.48 PM.png (2×1 px, 794 KB)
  • Next, I observed that we set snapPoints based on the value of contentHeight. To see if this includes handleComponent, I tested forcing snapPoints={[1]}. We can see that part of the handle component is hidden:
    Screenshot 2024-11-04 at 3.51.44 PM.png (2×1 px, 542 KB)
  • From this we can conclude that contentHeight needs to include some offset to make up for part of the height of the handle component. I did some testing of snapPoints values and found that we need exactly 24 pixels here.
  • From looking at the BottomSheetHandle component, I suspect this comes from (gap height) + (knobHandleContainer marginTop) + (knobHandle height), which adds up to 24

I'll put up a stack to fix this.

I put up a stack of diffs here: D13871

Please rebase on those diffs and remove the hardcoded paddingBottom: 24 hack

This revision is now accepted and ready to land.Fri, Nov 8, 5:27 AM