-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Conversation
Codecov ReportAttention: Patch coverage is
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
... and 18 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
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.
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
Snapshot / Visual test
|
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.
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! 👏
* @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
* @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
…into alignment-composable Rename path to objectPath for injection
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.
I don't think this is working properly yet. Here's a screenshot of a Time Strip using a synthed plan and some SWGs:

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.
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.
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
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.
From a visual/functional standpoint tested with synthetic plan in Gantt view, same plan added individually, and sine wave generators: LGTM!
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
Updated the following components to use the new composable
All Submissions:
Author Checklist
type:
label? Note: this is not necessarily the same as the original issue.Reviewer Checklist