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(#6812): Align Plot and Plan X-Axes in Time Strips #7744

Merged
merged 26 commits into from
Jul 22, 2024
Merged

Conversation

shefalijoshi
Copy link
Contributor

@shefalijoshi shefalijoshi commented Jun 11, 2024

Closes #6812

Describe your changes:

Remove use of props to share tick widths between components
Use a composable to manage alignment state of left, right ticks for plots instead

  • Parents register alignment composables with the path
  • Children update alignment based on the path as well
  • Y axes listen for changes to alignment and apply those as computed properties to adjust left/right styles

Updated the following components to use the new composable

  • Plots
  • Stacked Plots
  • Plans
  • Time strip
  • Time system axis

All Submissions:

  • Have you followed the guidelines in our Contributing document?
  • Have you checked to ensure there aren't other open Pull Requests for the same update/change?
  • Is this a notable change that will require a special callout in the release notes? For example, will this break compatibility with existing APIs or projects that consume these plugins?

Author Checklist

  • Changes address original issue?
  • Tests included and/or updated with changes?
  • Has this been smoke tested?
  • Have you associated this PR with a type: label? Note: this is not necessarily the same as the original issue.
  • Have you associated a milestone with this PR? Note: leave blank if unsure.
  • Testing instructions included in associated issue OR is this a dependency/testcase change?

Reviewer Checklist

  • Changes appear to address issue?
  • Reviewer has tested changes by following the provided instructions?
  • Changes appear not to be breaking changes?
  • Appropriate automated tests included?
  • Code style and in-line documentation are appropriate?

src/ui/composables/alignmentContext.js Fixed Show resolved Hide resolved
src/ui/composables/alignmentContext.js Fixed Show fixed Hide fixed
src/ui/composables/alignmentContext.js Fixed Show fixed Hide fixed
Copy link

codecov bot commented Jun 11, 2024

Codecov Report

Attention: Patch coverage is 80.43478% with 27 lines in your changes missing coverage. Please review.

Project coverage is 56.75%. Comparing base (7627629) to head (548533f).

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #7744      +/-   ##
==========================================
- Coverage   57.09%   56.75%   -0.34%     
==========================================
  Files         673      674       +1     
  Lines       27225    27273      +48     
  Branches     2663     2665       +2     
==========================================
- Hits        15545    15480      -65     
- Misses      11341    11452     +111     
- Partials      339      341       +2     
Flag Coverage Δ
e2e-full 23.51% <2.77%> (-18.37%) ⬇️
e2e-stable 60.73% <100.00%> (+0.06%) ⬆️
unit 49.38% <77.53%> (-0.08%) ⬇️
Files Coverage Δ
...gins/inspectorDataVisualization/TelemetryFrame.vue 0.00% <ø> (ø)
src/plugins/plot/MctTicks.vue 78.26% <100.00%> (+2.07%) ⬆️
src/plugins/plot/PlotView.vue 69.23% <ø> (-1.14%) ⬇️
src/plugins/plot/PlotViewProvider.js 96.15% <ø> (ø)
src/plugins/plot/chart/MctChart.vue 50.21% <ø> (ø)
...lugins/plot/overlayPlot/OverlayPlotViewProvider.js 94.44% <ø> (ø)
src/plugins/plot/stackedPlot/StackedPlot.vue 62.88% <100.00%> (+0.98%) ⬆️
src/plugins/plot/stackedPlot/StackedPlotItem.vue 37.31% <ø> (+1.08%) ⬆️
...lugins/plot/stackedPlot/StackedPlotViewProvider.js 94.44% <ø> (ø)
...gins/plot/stackedPlot/mixins/objectStyles-mixin.js 84.31% <ø> (ø)
... and 7 more

... and 18 files with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7627629...548533f. Read the comment docs.

Copy link
Member

@ozyx ozyx left a comment

Choose a reason for hiding this comment

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

Kinda half way through a review right now, but I decided to smoke test and I'm still seeing misalignment--

Screen.Recording.2024-06-18.at.10.35.46.AM.mov

src/plugins/plan/components/ActivityTimeline.vue Outdated Show resolved Hide resolved
src/plugins/timeline/TimelineViewLayout.vue Outdated Show resolved Hide resolved
src/ui/composables/alignmentContext.js Outdated Show resolved Hide resolved
@shefalijoshi shefalijoshi requested a review from ozyx June 20, 2024 20:02
@shefalijoshi shefalijoshi added bug:visual Visual problem. Not a functional issue type:bug and removed bug:visual Visual problem. Not a functional issue labels Jun 20, 2024
@shefalijoshi
Copy link
Contributor Author

Snapshot / Visual test

  • Test suite for time strip/ time line test with Gantt chart and add telemetry with 2 left and 1 right y-axes
  • Large time conductor bounds
  • ITC on and off

Copy link
Member

@ozyx ozyx left a comment

Choose a reason for hiding this comment

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

Firstly, awesome work here! I think what you've done here is a really great application of using composables to share state between components. I only have a few suggestions, but I've tested this locally with plans, gantts, timelines (with plans, gantts, and plots) and the timeline seems to be adjusted correctly! 👏

src/plugins/plot/MctPlot.vue Show resolved Hide resolved
src/plugins/plot/MctTicks.vue Outdated Show resolved Hide resolved
src/plugins/plot/MctTicks.vue Outdated Show resolved Hide resolved
src/plugins/plot/axis/YAxis.vue Outdated Show resolved Hide resolved
src/plugins/plot/axis/YAxis.vue Show resolved Hide resolved
src/plugins/timeline/TimelineViewProvider.js Show resolved Hide resolved
src/plugins/timeline/TimelineViewLayout.vue Show resolved Hide resolved
src/ui/composables/alignmentContext.js Outdated Show resolved Hide resolved
src/ui/composables/alignmentContext.js Outdated Show resolved Hide resolved
src/ui/composables/alignmentContext.js Fixed Show resolved Hide resolved
* @param {RemoveParams} param0 - The object containing yAxisId and updateObjectPath.
*/
const remove = ({ yAxisId, updateObjectPath } = {}) => {
const key = getAlignmentKeyForPath(updateObjectPath);

Check warning

Code scanning / CodeQL

Superfluous trailing arguments Warning

Superfluous argument passed to
function getAlignmentKeyForPath
.
* @param {UpdateParams} param0 - The object containing width, yAxisId, and updateObjectPath.
*/
const update = ({ width, yAxisId, updateObjectPath } = {}) => {
const key = getAlignmentKeyForPath(updateObjectPath);

Check warning

Code scanning / CodeQL

Superfluous trailing arguments Warning

Superfluous argument passed to
function getAlignmentKeyForPath
.
@ozyx ozyx added this to the Target:4.0.0 milestone Jun 25, 2024
@shefalijoshi shefalijoshi requested a review from ozyx June 26, 2024 21:36
@akhenry akhenry requested a review from charlesh88 June 27, 2024 17:08
Copy link
Contributor

@charlesh88 charlesh88 left a comment

Choose a reason for hiding this comment

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

I don't think this is working properly yet. Here's a screenshot of a Time Strip using a synthed plan and some SWGs:

Screenshot 2024-06-27 at 12 47 38 PM

Time Conductor is set to realtime, 30 secs into future and 30 secs into the past. I'd expect the data display area extents to be exactly the same to the left and right of the "now" line; this is shown with the green arrows and lines. But instead, the area to the left of now is truncated by the width needs of the plot Y axis labels; this is shown with the orange line and arrow. Note that the UTC strip at top is not truncated.

This leads me to believe that the data is not being aligned properly relative to time, as shown in the UTC lane. It's unclear to me if data is aligning with each other independently of time or not (Plan activities and numeric telemetry SWGs). Even if this is true, the display should still be showing me an equal span of data to either side of the now line given these current settings.

Let me know if this is unclear or if I'm missing something.

@shefalijoshi
Copy link
Contributor Author

shefalijoshi commented Jul 1, 2024

I don't think this is working properly yet. Here's a screenshot of a Time Strip using a synthed plan and some SWGs:
Screenshot 2024-06-27 at 12 47 38 PM

Time Conductor is set to realtime, 30 secs into future and 30 secs into the past. I'd expect the data display area extents to be exactly the same to the left and right of the "now" line; this is shown with the green arrows and lines. But instead, the area to the left of now is truncated by the width needs of the plot Y axis labels; this is shown with the orange line and arrow. Note that the UTC strip at top is not truncated.

This leads me to believe that the data is not being aligned properly relative to time, as shown in the UTC lane. It's unclear to me if data is aligning with each other independently of time or not (Plan activities and numeric telemetry SWGs). Even if this is true, the display should still be showing me an equal span of data to either side of the now line given these current settings.

Let me know if this is unclear or if I'm missing something.

Good catch.
I've pushed a fix for this. The now marker was missing the alignment adjustment.

Copy link
Member

@ozyx ozyx left a comment

Choose a reason for hiding this comment

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

Looks good from a smoke-test / visual standpoint. Tested with a timeline containing a plan, gantt chart (with same plan) and a swg.

Still have some failing tests that need to be fixed, and any additional test coverage we can add

Copy link
Contributor

@charlesh88 charlesh88 left a comment

Choose a reason for hiding this comment

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

From a visual/functional standpoint tested with synthetic plan in Gantt view, same plan added individually, and sine wave generators: LGTM!

@shefalijoshi shefalijoshi requested a review from ozyx July 22, 2024 20:49
@ozyx ozyx changed the title Alignment composable fix(#6812): Align Plot and Plan X-Axes in Time Strips Jul 22, 2024
@ozyx ozyx added the pr:e2e:couchdb npm run test:e2e:couchdb label Jul 22, 2024
@github-actions github-actions bot removed the pr:e2e:couchdb npm run test:e2e:couchdb label Jul 22, 2024
@ozyx ozyx merged commit 1fae0a6 into master Jul 22, 2024
23 of 24 checks passed
@ozyx ozyx deleted the alignment-composable branch July 22, 2024 23:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Plots in Time Strip are not aligning properly with Plans
4 participants