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

fix: [GRWT-3474] Point_Highlight_Enlarges_when_navigating_back_to_same_chart_view #349

Conversation

ilya-deriv
Copy link
Contributor

https://app.clickup.com/t/20696747/GRWT-3474

This PR contains the following changes:

  • ✨ New feature (non-breaking change which adds functionality)
  • 🛠️ Bug fix (non-breaking change which fixes an issue)
  • ❌ Breaking change (fix or feature that would cause existing functionality to change)
  • 🧹 Code refactor
  • ✅ Build configuration change
  • 📝 Documentation
  • 🗑️ Chore

Developers Note (Optional)

Pre-launch Checklist (For PR creator)

As a creator of this PR:

  • ✍️ I have included clickup id and package/app_name in the PR title.
  • 👁️ I have gone through the code and removed any temporary changes (commented lines, prints, debug statements etc.).
  • ⚒️ I have fixed any errors/warnings shown by the analyzer/linter.
  • 📝 I have added documentation, comments and logging wherever required.
  • 🧪 I have added necessary tests for these changes.
  • 🔎 I have ensured all existing tests are passing.

Reviewers

@ramin-deriv

Pre-launch Checklist (For Reviewers)

As a reviewer I ensure that:

  • ✴️ This PR follows the standard PR template.
  • 🪩 The information in this PR properly reflects the code changes.
  • 🧪 All the necessary tests for this PR's are passing.

Pre-launch Checklist (For QA)

  • 👌 It passes the acceptance criteria.

Pre-launch Checklist (For Maintainer)

  • [MAINTAINER_NAME] I make sure this PR fulfills its purpose.

Vector _vector = const Vector.zero();

/// Keeps the latest position of the start and end point of drawing
Point? _startPoint, _endPoint;

/// Marker full size
double markerFullSize = 10;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this going to change how line drawing will look like on the web, if so, we should consider not affecting the web.
either by giving customization options, and having default values as something that keeps the appearance of the drawing tool
or other options it to create our mobile version line drawing class which is preferred. if we see the behavior of it is too much different from the web implementation

Comment on lines +279 to +283
final LineDrawingToolLabelPainter? _lineDrawingToolLabelPainter =
lineConfig.getLabelPainter(
startPoint: _startPoint!,
endPoint: _endPoint!,
);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't we create an instance of MobileLineDrawingToolLabelPainter here?

 final LineDrawingToolConfigMobile lineConfig = config as LineDrawingToolConfigMobile;

    if (_startPoint == null || _endPoint == null) {
      return;
    }

    if (drawingData.isSelected && drawingData.isDrawingFinished) {
      final MobileLineDrawingToolLabelPainter? _lineDrawingToolLabelPainter =
          MobileLineDrawingToolLabelPainter(
        lineConfig,
        startPoint: _startPoint!,
        endPoint: _endPoint!,
      );

      _lineDrawingToolLabelPainter?.paint(canvas, size, chartConfig, epochFromX,
          quoteFromY, epochToX, quoteToY);

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think for the web version which is LineDrawing class, the labelPainter was being returned as null.
so we didn't paint anything in line _lineDrawingToolLabelPainter?.paint(canvas, size, chartConfig, epochFromX, quoteFromY, epochToX, quoteToY); because it was null. then we can remove it. and onLabelPaint in LineDrawing will become:

  @override
  void onLabelPaint(
    Canvas canvas,
    Size size,
    ChartTheme theme,
    ChartConfig chartConfig,
    int Function(double x) epochFromX,
    double Function(double) quoteFromY,
    double Function(int x) epochToX,
    double Function(double y) quoteToY,
    DrawingToolConfig config,
    DrawingData drawingData,
    DataSeries<Tick> series,
  ) {}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually it's not a mandator override, in LineDrawing we can just remove the onLabelPaint method override, doesn't have to have empty override

@ilya-deriv ilya-deriv force-pushed the Point_Highlight_Enlarges_when_navigating_back_to_same_chart_view branch from 65cdaa4 to 15feae0 Compare December 16, 2024 10:32
@ramin-deriv ramin-deriv merged commit 514b654 into deriv-com:master Dec 19, 2024
2 checks passed
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.

3 participants