-
-
Notifications
You must be signed in to change notification settings - Fork 723
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
Improve rotate experience when close to center #5104
Conversation
Codecov ReportAttention: Patch coverage is
❌ We are unable to process any of the uploaded JUnit XML files. Please ensure your files are in the right format.
Additional details and impacted files@@ Coverage Diff @@
## main #5104 +/- ##
==========================================
- Coverage 91.81% 91.81% -0.01%
==========================================
Files 279 278 -1
Lines 38347 38318 -29
Branches 6702 6703 +1
==========================================
- Hits 35209 35181 -28
+ Misses 3006 3004 -2
- Partials 132 133 +1 ☔ View full report in Codecov by Sentry. |
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.
Works much better around the center!
I'm still not a fan of this control scheme as a whole though, I can't intuitively predict how will mouse movements translate to map rotation, even after playing with the map for a while and reading about how the new rotation controls work.
I think only you and I have rotated the map on the desktop and are aware of this issue :-) otherwise I would've expected other people to complain about the behavior... |
I see now that coverage is a bit lacking, I'll see if I can solve that. |
@kubapelc let me know if you want to review this again, I've changed the navigation control heavily... |
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.
I agree with @kubapelc that the handling is not great via mouse because you don't get an intuition of what will happen but this is an improvement over what we have currently.
Added a couple of minor comments but approving either way
@@ -84,7 +84,7 @@ describe('NavigationControl', () => { | |||
const navRect = navButton.getClientRects(); | |||
|
|||
const buttonX = (navRect.x ?? 0) + (navRect.width ?? 0) / 2; | |||
const buttonY = (navRect.y ?? 0) + (navRect.height ?? 0) / 2; | |||
const buttonY = (navRect.y ?? 0) + (navRect.height ?? 0) / 2 - 1; |
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.
What's this -1
doing here?
Can we add a comment so it's clear what it's doing?
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.
Instead of dragging from the exact center of the button where it won't change the rotation I moved the drag to be one pixel lower.
@@ -168,7 +168,7 @@ describe('NavigationControl', () => { | |||
const navRect = navButton.getClientRects(); | |||
|
|||
const buttonX = (navRect.x ?? 0) + (navRect.width ?? 0) / 2; | |||
const buttonY = (navRect.y ?? 0) + (navRect.height ?? 0) / 2; | |||
const buttonY = (navRect.y ?? 0) + (navRect.height ?? 0) / 2 - 1; |
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.
Same here
Launch Checklist
This is a continue of the following PR, where a comment mention that the experience around the center it not great.
I have reverted to experience around the center to be as it was before my change.
Up until 100 pixels from the center.
Simplified the navigation control to use a single handler.
CHANGELOG.md
under the## main
section.