-
Couldn't load subscription status.
- Fork 22
feat: Accumulators markers revamp #435
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: Accumulators markers revamp #435
Conversation
Reviewer's guide (collapsed on small PRs)Reviewer's GuideRefactors the accumulator marker icon painter by removing the previous tick barrier feature and cleaning up related parameters and enum definitions. Class diagram for refactored AccumulatorMarkerIconPainter and ChartMarkerclassDiagram
class AccumulatorMarkerIconPainter {
+paintMarkers()
-_drawPreviousTickBarrier(size, canvas, startX, endX, y, barrierColor)
}
class TickMarkerIconPainter
AccumulatorMarkerIconPainter --|> TickMarkerIconPainter
class ChartMarker
class MarkerType
MarkerType : latestTick
MarkerType : highBarrier
MarkerType : lowBarrier
MarkerType : exitSpot
MarkerType : tick
%% Removed enum value
%% MarkerType : previousTick (removed)
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
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 there - I've reviewed your changes - here's some feedback:
- Since the previousTick marker was fully removed, make sure to clean up or update any remaining references to MarkerType.previousTick elsewhere in the codebase for consistency.
- The accumulator painter’s paint method still handles multiple drawing responsibilities—consider extracting the barrier and shape rendering into smaller helper methods for better readability and testability.
- Now that _drawPreviousTickBarrier only uses a single color, rename the method or its parameters to reflect that it’s no longer tied to a distinct ‘previous tick’ color for clarity.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Since the previousTick marker was fully removed, make sure to clean up or update any remaining references to MarkerType.previousTick elsewhere in the codebase for consistency.
- The accumulator painter’s paint method still handles multiple drawing responsibilities—consider extracting the barrier and shape rendering into smaller helper methods for better readability and testability.
- Now that _drawPreviousTickBarrier only uses a single color, rename the method or its parameters to reflect that it’s no longer tied to a distinct ‘previous tick’ color for clarity.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
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
Revamp accumulator marker icons by removing previous tick marker support and simplifying barrier drawing
Enhancements: