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

Refactor plots to use plotly.js instead of chart.js (part of DSEGOG-57) #564

Merged
merged 16 commits into from
Feb 6, 2025

Conversation

louise-davies
Copy link
Member

Description

Motivation for this change is DSEGOG-57 - Plotly has the rangebreaks feature which can implement the skip non-working hours feature. That change will be in a follow up PR to try and keep this PR smaller as it's already a behemoth!

Overall the plots should work identically to before - the only difference I can note is that we can't easily properly limit the zoom/pan range to the range of the data anymore. But since that only affects zoom/panning I decided it's not that big of a deal as the user can just choose to keep within bounds or not.

I had to rearrange some of the styling in the windows to accomodate Plotly to ensure the plot is correctly responsive and fills up the container as it is supposed to.

Also, some minor improvements in the playwright tests - we now use a better screenshot method (it takes two screenshots and if they're not the same it will continue taking subsequent screenshots until the page is stable before comparing). Also changed it so that Webkit screenshots are no longer huge - this is due to the default DPI of Webkit being 2 instead of 1.

Testing instructions

  • Review code
  • Check Actions build
  • Review changes to test coverage
  • Test plotting, traces & image crosshairs functionality to ensure migration has not lost any functionality

Agile board tracking

connected to DSEGOG-57

- WIP, still need to migrate traces
- also remove some chart.js references
Also update CDN url for plotly to more optimised version
- fix some issues with the plotting code & styling
- also use better screenshot command & make webkit snapshots not as large
- mostly just accounting for the webkit change & switching to better screenshot method
- Had to change some layout stuff to make things resize properly/show the right size in tests
Copy link

codecov bot commented Jan 31, 2025

Codecov Report

Attention: Patch coverage is 99.86413% with 1 line in your changes missing coverage. Please review.

Project coverage is 97.64%. Comparing base (1ec2bf0) to head (00fd021).
Report is 17 commits behind head on develop.

Files with missing lines Patch % Lines
src/windows/thumbnailSelector.component.tsx 99.05% 1 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           develop     #564      +/-   ##
===========================================
- Coverage    97.64%   97.64%   -0.01%     
===========================================
  Files           97       97              
  Lines        12463    12426      -37     
  Branches      2043     2071      +28     
===========================================
- Hits         12170    12133      -37     
  Misses         287      287              
  Partials         6        6              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@louise-davies louise-davies marked this pull request as ready for review February 4, 2025 11:40
Copy link
Contributor

@joshdimanteto joshdimanteto left a comment

Choose a reason for hiding this comment

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

Works really well. Just some minor comments

I expected the data to be cleared when the user selects the XY as this is the current functionality on dev.

bug_1.mp4

DEV

dev_bug_1.mp4

- bug was that when switching from timeseries plot to XY plot, the plot assumed it was still a timeseries plot instead of drawing a blank plot until the user selects an XAxis
@louise-davies
Copy link
Member Author

@joshuadkitenge good catch! Should be fixed now - I wasn't handling the case of XAxis being undefined properly.

joshdimanteto
joshdimanteto previously approved these changes Feb 5, 2025
Copy link
Contributor

@joshdimanteto joshdimanteto left a comment

Choose a reason for hiding this comment

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

LGTM

@louise-davies louise-davies merged commit 0fe0d77 into develop Feb 6, 2025
6 checks passed
@louise-davies louise-davies deleted the refactor-plots branch February 6, 2025 15:25
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