-
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
Add Independent Time Conductor to ImageView #6496
Add Independent Time Conductor to ImageView #6496
Conversation
…uld-support-showing-more-images-than-the-conductor-bounds
@charlesh88 If you'd like to take a crack at styling, here's the PR 😀 |
Note I also added independent time to Screen.Recording.2023-03-23.at.9.15.15.AM.mov |
Codecov Report
@@ Coverage Diff @@
## master #6496 +/- ##
==========================================
- Coverage 54.84% 54.77% -0.07%
==========================================
Files 627 627
Lines 26710 26718 +8
Branches 2418 2418
==========================================
- Hits 14649 14636 -13
- Misses 11397 11417 +20
- Partials 664 665 +1
*This pull request uses carry forward flags. Click here to find out more.
... and 7 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
Looking good, going in to do some needed finessing on the ITC. @charlesh88 TODOS
|
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.
Functionally this LGTM.
That said, inclusion of the ITC as it is currently takes up a great deal of incremental space, and would have a significant negative impact on existing displays as it is. As a short-term solution this approach is better than nothing, but needs a bunch of refinement in the CSS department (@charlesh88 to do) if we need this feature quickly in the short term; ideally if not, then this work should be brought over and combined with the WIP in branch time-conductor-4975
which was shelved last year and solves for the space problem cited earlier here.
…e-images-than-the-conductor-bounds
…e-images-than-the-conductor-bounds
I wonder if the underlying TelemetryCollection work can go in, and just exclude the rather small change to enable the time conductor on the various views. The e2e tests will probably need to remain in limbo. |
Talked with @akhenry and we'll split the backend work from the frontend work. I'll make a new PR for just the backend stuff. |
Closing this for the time being and making a new one to add backend work. |
@scottbell Let's revive this changeset and rebase it on the new time conductor branch. |
Closes #6434
Describe your changes:
Adds independent time conductor to the
ImageView
. Adds time context toTelemetryCollection
.I also added it to
DisplayLayout
, though this can be backed out if we're worried about risk. It appears to work though!All Submissions:
Author Checklist
Reviewer Checklist