Skip to content
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

#119 Tempered TextAnnotatorPopup positioning #120

Conversation

oleksandr-danylchenko
Copy link
Contributor

@oleksandr-danylchenko oleksandr-danylchenko commented Jul 12, 2024

Issue - #119

Changes Made

  • Added stable top positioning of the popup. It prevents it from covering the annotated content and it's placed pretty intuitively (IMHO)
  • Allowed the popup to get scrolled from to prevent it from dragging across the screen over irrelevant text
  • Added useDismiss hook usage

Relations

It prevents the page from accidentally freezing when the user selects the text with the Ctrl+A (bug I discovered when working on the #118, see

Demo

Screen.Recording.2024-07-12.at.19.33.39.mov

@rsimon
Copy link
Member

rsimon commented Jul 14, 2024

I like the change. But what if the top of the text is scrolled off screen, and the user selects? Right now, the popup would still pop up above the text, outside the visible area, right? I think it's important that the popup will always appear in the visible area - either above or below the text and, at worst, if the selection exceeds both the top and the bottom of the screen, above the annotation. (I guess there are behaviors in floating UI that would achieve that?)

@oleksandr-danylchenko
Copy link
Contributor Author

But what if the top of the text is scrolled off the screen, and the user selects? Right now, the popup would still pop up above the text, outside the visible area, right?

Yep, that's correct. It's possible to scroll from the popup and make it non-visible. I believe it can be improved with the flip middleware that will place the popup under the selected text on the scroll. Like it's done in Slab:

Screen.Recording.2024-07-18.at.10.46.02.mov

I think it's important that the popup will always appear in the visible area

I'm not sure whether the popup should still be visible if the user explicitly scrolled from the selected content. Because it removes the visual connection between them 🤔

@rsimon
Copy link
Member

rsimon commented Jul 18, 2024

The issue we face is this: there are sometimes annotations that extend beyond multiple paragraphs, and are larger than the viewport area.

If the user clicks an annotation that starts above the screen viewport, and ends below the screen viewport, they won't get a popup at all. (Because it would either be above or below the visible screen area.) That's what we need to avoid in our case.

I don't mind if the user actively scrolls the popup out of view. But clicking an annotation always has to open a popup inside the visible viewport area IMO.

@oleksandr-danylchenko
Copy link
Contributor Author

clicks an annotation that starts above the screen viewport, and ends below the screen viewport, they won't get a popup at all

Aha... I see your point now. I'll try to experiment with the shift middleware then 👌🏻

@oleksandr-danylchenko
Copy link
Contributor Author

oleksandr-danylchenko commented Jul 18, 2024

I found that the most pragmatic solution would be using just the shift({ mainAxis: false, crossAxis: true, padding: 10 }). It doesn't introduce weird jumping when the selection area is large, as the flip does. And it fulfills the need of having the popup visible for the large references.

Screen.Recording.2024-07-18.at.11.21.06.mov

@rsimon
Copy link
Member

rsimon commented Jul 18, 2024

YES! I love it!

@oleksandr-danylchenko oleksandr-danylchenko force-pushed the #119-fix-jittering-positioning-of-popup branch from 6fcc330 to 38a607f Compare July 18, 2024 11:59
@rsimon rsimon merged commit 6f8a24c into recogito:main Jul 19, 2024
@oleksandr-danylchenko oleksandr-danylchenko deleted the #119-fix-jittering-positioning-of-popup branch July 19, 2024 11:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants