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

Add Independent Time Conductor to ImageView #6496

Conversation

scottbell
Copy link
Contributor

@scottbell scottbell commented Mar 23, 2023

Closes #6434

Describe your changes:

Adds independent time conductor to the ImageView. Adds time context to TelemetryCollection.
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:

  • 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 change backwards compatible? For example, developers won't need to change how they are calling the API or how they've extended core plugins such as Tables or Plots.

Author Checklist

  • Changes address original issue?
  • Tests included and/or updated with changes?
  • Command line build passes?
  • Has this been smoke tested?
  • 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?
  • Has associated issue been labelled unverified? (only applicable if this PR closes the issue)
  • Has associated issue been labelled bug? (only applicable if this PR is for a bug fix)

@scottbell scottbell self-assigned this Mar 23, 2023
@scottbell
Copy link
Contributor Author

@charlesh88 If you'd like to take a crack at styling, here's the PR 😀

@scottbell
Copy link
Contributor Author

Note I also added independent time to DisplayLayout, and it appears to work! It's easy to revert if we think it's too much risk though:

Screen.Recording.2023-03-23.at.9.15.15.AM.mov

@codecov
Copy link

codecov bot commented Mar 23, 2023

Codecov Report

Merging #6496 (affcf70) into master (7ec2c44) will decrease coverage by 0.07%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            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     
Flag Coverage Δ *Carryforward flag
e2e-ci 39.39% <ø> (+0.02%) ⬆️ Carriedforward from 7ec2c44
e2e-full 51.05% <ø> (-0.03%) ⬇️ Carriedforward from 7ec2c44
e2e-stable 56.02% <100.00%> (+0.50%) ⬆️
unit 49.08% <70.58%> (-0.11%) ⬇️

*This pull request uses carry forward flags. Click here to find out more.

Impacted Files Coverage Δ
src/ui/components/ObjectView.vue 46.11% <ø> (-1.56%) ⬇️
example/imagery/plugin.js 90.47% <100.00%> (+0.23%) ⬆️
src/api/telemetry/TelemetryAPI.js 85.13% <100.00%> (+0.05%) ⬆️
src/api/telemetry/TelemetryCollection.js 83.69% <100.00%> (+0.45%) ⬆️
src/plugins/imagery/mixins/imageryData.js 93.07% <100.00%> (ø)

... and 7 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 7ec2c44...affcf70. Read the comment docs.

@scottbell scottbell requested a review from unlikelyzero March 23, 2023 11:09
@charlesh88
Copy link
Contributor

Looking good, going in to do some needed finessing on the ITC.

@charlesh88 TODOS

  • Hide ITC when its view's frame is hidden in a layout.
  • Make toggle switch, icon and buttons smaller.
  • Better vertical alignment of elements.

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.

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.

@scottbell
Copy link
Contributor Author

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.

@scottbell
Copy link
Contributor Author

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.

@scottbell
Copy link
Contributor Author

Closing this for the time being and making a new one to add backend work.

@akhenry
Copy link
Contributor

akhenry commented Jul 10, 2023

@scottbell Let's revive this changeset and rebase it on the new time conductor branch.

@scottbell scottbell deleted the 6434-image-view-should-support-showing-more-images-than-the-conductor-bounds branch November 30, 2023 08:31
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.

Image view should support showing more images than the conductor bounds
3 participants