-
Notifications
You must be signed in to change notification settings - Fork 12
feat: Jim/GRWT-5290/update crosshair UI based on design #374
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
base: master
Are you sure you want to change the base?
feat: Jim/GRWT-5290/update crosshair UI based on design #374
Conversation
Reviewer's GuideThis PR overhauls the chart’s crosshair feature by introducing a unified CrosshairController and widget pipeline in the interactive layer, supporting two visual variants (smallScreen/largeScreen). It integrates crosshair gestures into the custom gesture system, extends series APIs to provide crosshair info and highlight painters, and adds new UI components (CrosshairArea, CrosshairDetails) and utilities (ChartDateUtils, axis ranges, text styles) to render configurable crosshair UIs. Example, theme, and test code have been updated accordingly. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
@sourcery-ai review |
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.
Hey @jim-deriv - I've reviewed your changes - here's some feedback:
- Consider a more unified widget strategy for responsive crosshair information in series to minimize potential code duplication.
- Verify if dynamic animation speeds for the crosshair are intended, as
_dragVelocity
inCrosshairController
currently results in a fixed 5ms duration. - Clarify the intended positioning behavior for the crosshair details box in
CrosshairArea
, as the current code primarily places it above the cursor, differing from a comment suggesting conditional placement.
Here's what I looked at during the review
- 🟡 General issues: 1 issue found
- 🟢 Security: all looks good
- 🟡 Review instructions: 3 issues found
- 🟢 Testing: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
lib/src/deriv_chart/interactive_layer/crosshair/crosshair_controller.dart
Outdated
Show resolved
Hide resolved
lib/src/deriv_chart/interactive_layer/crosshair/crosshair_line_painter.dart
Outdated
Show resolved
Hide resolved
lib/src/deriv_chart/interactive_layer/crosshair/crosshair_controller.dart
Outdated
Show resolved
Hide resolved
@sourcery-ai review |
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.
Hey @jim-deriv - I've reviewed your changes - here's some feedback:
- The
currentCrosshairTick
parameter added to numerous painter methods enables highlighting; verify consistent application of highlighting effects where this parameter is now available. - The new auto-panning behavior in
CrosshairController
during long-press interactions should be tested for smooth integration with existing chart navigation. - The
CrosshairController
's dynamic animation duration based on drag velocity is a fine-grained control; ensure it provides a natural feel during crosshair movement.
Here's what I looked at during the review
- 🟢 General issues: all looks good
- 🟢 Security: all looks good
- 🟢 Review instructions: all looks good
- 🟢 Testing: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
7f75be8
to
e999624
Compare
e2a93e9
to
7454335
Compare
12ae3a0
to
bee755d
Compare
…grwt-5290/update-crosshair-ui-based-on-design
…grwt-5290/update-crosshair-ui-based-on-design
…hover_cross_hair_or_tools handle onHover and decide cross-hair or tools should take it
…grwt-5290/update-crosshair-ui-based-on-design
- Added FontFeature.proportionalFigures to resolve overlap issue.
…grwt-5290/update-crosshair-ui-based-on-design
Clickup link:
Fixes issue: #
This PR contains the following changes:
Developers Note (Optional)
Pre-launch Checklist (For PR creator)
As a creator of this PR:
Reviewers
Pre-launch Checklist (For Reviewers)
As a reviewer I ensure that:
Pre-launch Checklist (For QA)
Pre-launch Checklist (For Maintainer)
Summary by Sourcery
Add a full crosshair interface following design specs, introduce new gesture and interaction layers for drawing tools, and enhance chart rendering and testing.
New Features:
Enhancements:
Documentation:
Tests:
Chores: