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

In time conductor history, show them on hover if only milliseconds have changed #4386

Closed
shefalijoshi opened this issue Oct 27, 2021 · 14 comments · Fixed by #5783
Closed

In time conductor history, show them on hover if only milliseconds have changed #4386

shefalijoshi opened this issue Oct 27, 2021 · 14 comments · Fixed by #5783

Comments

@shefalijoshi
Copy link
Contributor

If there is only millisecond change between start and end time, the history shows the same times. Here
start: 2021-10-27 16:18:26.979Z
end: 2021-10-27 16:18:26.989Z

image

Originally posted by @shefalijoshi in #4197 (comment)

@mariuszr
Copy link
Contributor

mariuszr commented Oct 27, 2021

@shefalijoshi Could you please provide a bit more insight on the meaning on hover? Do you mean any tooltip when you hover the time?

In my option, we should follow the existing way of the day, minute, second presentation and adapt for the milliseconds, which means that + 1ms in the case of 1 millisecond.

@akhenry
Copy link
Contributor

akhenry commented Aug 15, 2022

Test Instructions

  1. Set the time conductor to fixed mode
  2. Set the following bounds:
  • start: 2021-10-27 16:18:26.100Z
  • end: 2021-10-27 16:18:26.200Z
  1. Now change the bounds to the following:
  • start: 2021-10-27 16:18:26.300Z
  • end: 2021-10-27 16:18:26.400Z
  1. Verify that both sets of bounds appear in the time conductor history popup.
  2. Hover over each entry and verify that the MS are shown on hover, allowing the user to differentiate between the apparently duplicate entries.

@jvigliotta
Copy link
Contributor

I think this maybe implemented differently from suggested, but I do think it may be the best way to do it. The way it's implemented, then MS will be shown, but in the new format, it should be much more readable. If we were to only show on hover, that would require adding functionality to the MenuAPI and I don't think it's the correct fix, as we'd still have duplicates in the history menu at first glance. If it's formatted in a nicer, easier to understand way, then I think we should be good and I think this PR does that.

@ozyx
Copy link
Member

ozyx commented Sep 12, 2022

Verified Testathon 9/12/2022.

The history was populated correctly, and switching between the different timestamps worked as expected.

History showed timestamp and a + ms as such:

image

On hover, the full timestamp was NOT shown, however:

image

One potential issue I noticed is that there is no way to tell between two history entries with the same HH:mm:ss timestamp and millisecond diff (i.e. 100 ms - 200 ms vs. 400 - 500 ms). Not sure how big of an impact this will be, but figured this was important to note.

@rukmini-bose
Copy link
Contributor

Testathon 09/12 Notes:

  • Steps 1-4 work as expected.
  • Step 5 UNVERIFIED: On hover, ms are NOT shown.

@khalidadil
Copy link
Contributor

I see the same thing as @rukmini-bose

@jvigliotta
Copy link
Contributor

@rukmini-bose @khalidadil Yeah, we didn't implement that. the reason being, we can't have the same "name" in a menu and that's what would happen with same timestamps (only showing ms on hover). This is a different implementation that addresses the same issue.

@unlikelyzero
Copy link
Collaborator

@charlesh88 assigning to Charles to verify that the implementation matches intended design

@charlesh88
Copy link
Contributor

Testathon 9/15/22: I think same as reported by Rukmini on Monday - don't see MS on hover, but should see them. So unverified (?).

image

@unlikelyzero unlikelyzero reopened this Sep 15, 2022
@unlikelyzero
Copy link
Collaborator

Re-opening:

This implementation is great, but we need to have milliseconds appear on mouse hover.

@mariuszr
Copy link
Contributor

@jvigliotta, @unlikelyzero, @charlesh88, @akhenry My apologies. I think that a bit discrepancies between the issue title and description that was also completed in the comment #4414 (comment) however not included in the issue description caused the incorrect defining the scope of required changes.

Please see a new PR 5783

@shefalijoshi
Copy link
Contributor Author

Verified that ms are displayed on hover.

@jvigliotta
Copy link
Contributor

@mariuszr No worries! I was a bit mixed up on this one as well, but it's looking great now. Nice work!

@rukmini-bose
Copy link
Contributor

Verified Testathon 9/30/22.
Looks great!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
9 participants