Page MenuHomePhabricator

Allow for tooltip to be hidden when revealing spoiler while maintaining styles
AbandonedPublic

Authored by rohan on Nov 18 2022, 3:03 PM.
Tags
None
Referenced Files
Unknown Object (File)
Sun, Oct 27, 2:16 PM
Unknown Object (File)
Oct 13 2024, 2:21 AM
Unknown Object (File)
Oct 6 2024, 7:23 AM
Unknown Object (File)
Oct 6 2024, 4:17 AM
Unknown Object (File)
Sep 27 2024, 8:22 PM
Unknown Object (File)
Sep 27 2024, 8:22 PM
Unknown Object (File)
Sep 27 2024, 8:22 PM
Unknown Object (File)
Sep 27 2024, 8:21 PM
Subscribers

Details

Reviewers
atul
ginsu
Summary

Currently, in D5539, we introduce GestureTouchableOpacity in order to block the tooltip from appearing when we want to reveal the spoiler. There are several problems with this approach:

  1. Pressing a spoiler once it is already revealed still prevents the tooltip from appearing.
  2. More importantly, the styling is completely different to what is within the text message. The text size, color, and also the y-axis of the text is all unexpected (Context: https://linear.app/comm/issue/ENG-2272/gesturetouchableopacity-changing-styles).

Based on some experimenting, this allows us to have the two desired behaviors - the spoiler is revealed without showing the tooltip, and the styling once revealed is consistent.

I plan to resolve the need for the eslint ignore comment before landing, but I'm putting this diff up to allow for reviewers to repro locally to test, and also to figure out the reason for these changes fixing things. Speaking with @atul briefly, we think it may be some dependency handling fault from our end.

Depends on D5541

Test Plan

Demo video is attatched below. The test plan:

  1. Restart keyserver and native with yarn dev
  2. Simulate a spoiler reveal action by tapping on a spoiler
  3. Simulate a general tap on a message outside of spoilers to expect tooltip
  4. Simulate a tap on a spoiler (after revealed) to expect tooltip
  5. Simulate a long-press on a message to expect the tooltip

Diff Detail

Repository
rCOMM Comm
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

rohan requested review of this revision.Nov 18 2022, 3:18 PM
atul requested changes to this revision.Nov 21 2022, 3:18 PM

Is this ready for review? Sending back to your queue since I think this diff in this state was mostly for demonstration purposes, but feel free to re-request review if I'm misreading

This revision now requires changes to proceed.Nov 21 2022, 3:18 PM
In D5688#169584, @atul wrote:

Is this ready for review? Sending back to your queue since I think this diff in this state was mostly for demonstration purposes, but feel free to re-request review if I'm misreading

Oh yeah my bad - I think it's more logical to figure out the styling issues with GestureTouchableOpacity in D5539 if possible at the moment.

I think we should abandon this one, I'm not sure if it's reproducible

Agreed - I think resolving the issues in D5541 is the better way forward. Abandoning as this was mainly used to allow reviewers to repro locally easier anyways