-
Notifications
You must be signed in to change notification settings - Fork 12
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix highlighter behavior on mobile #1694
base: master
Are you sure you want to change the base?
Conversation
All highlighters would process these touch events, regardless of whether the events were meant for them or not
Helps learn when we misconfigured nri-ui examples
Also, we're guaranteed to have events only for the current highlighter, since this event listener is assigned to document.getElementById(id) up top.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good stuff.
Sounds like we have some holes in our testing process for mobile. 😬
I tested this on my Google Pixel 6 using Chrome, and confirmed that the bugs shown in spooky.action.at.a.distance.mov
and viewFold.mov
are fixed.
scroll.highlight.mov
is broken, as expected.
ps: i'm holding this one bc fixing persisting highlights on mobile while mobile is still so prone to accidental highlights is not super desirable, so I'll merge this one when #1695 is ready (ty for the speedy review, regardless!) |
🔧 Modifying a component
Context
Part of https://linear.app/noredink/issue/PHX-1796/investigate-double-click-to-highlight-individual-words#comment-3a9f6403
We noticed users were commonly creating highlights and deleting them within 5s, and deemed those "accidental highlights".
We suspected the accidental highlights were coming from mobile devices, and investiating it opened a bit of a pandora's box for mobile issues with highlighting. The ones relevant to noredink-ui are:
scroll.highlight.mov
spooky.action.at.a.distance.mov
viewFold
, because:viewFold.mov
This fixes the 2nd and 3rd issues. The first one will be harder, so I'll do it in a follow-up PR.
I chose not to make
viewFold
create a container, which might be a mistake, because it'll be easy for folks to forget that's necessary for mobile, like we forgot to set up highlighter ports at all in project Text Response. I'm super open to forcing a genericdiv
container insideviewFold
.Component completion checklist
nriDescription