Skip to content

Fix/time drag n drop#3

Merged
meganrm merged 5 commits intomasterfrom
fix/time-drag-n-drop
Jul 30, 2020
Merged

Fix/time drag n drop#3
meganrm merged 5 commits intomasterfrom
fix/time-drag-n-drop

Conversation

@meganrm
Copy link
Copy Markdown
Contributor

@meganrm meganrm commented Jul 30, 2020

Addressing : https://aicsjira.corp.alleninstitute.org/browse/AGENTVIZ-748

I didn't fully debug why it wasn't working before, but when I made the slider calculations simpler it worked.

I also formatted the playhead to have 3 digits and covert to units so it's not showing "2000k ns" for example.

Lastly, I changed the function call when you click on the slider, which was autoplaying instead of just going to the new time if the simulation is paused.

Future work: still need to do some deboucing probably to keep someone from just clicking all over the slider. Also, a loading state indicator would probably be good.

Pull request recommendations:

  • Name your pull request your-development-type/short-description. Ex: feature/read-tiff-files
  • Link to any relevant issue in the PR description. Ex: Resolves [gh-##], adds tiff file format support
  • Provide description and context of changes.
  • Provide relevant tests for your feature or bug fix.
  • Provide or update documentation for any feature added by your pull request.

Thanks for contributing!

@meganrm meganrm requested review from a user, blairlyons, evamaxfield and toloudis July 30, 2020 00:09
Comment thread package.json Outdated
"dependencies": {
"@aics/simularium-observables-manager": "^0.1.0",
"@aics/simularium-viewer": "^1.2.5",
"@aics/simularium-viewer": "file:../agentviz-viewer",
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

You probably don't want this to go to master, I'm guessing?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

It should already be updated.... try reloading.

@toloudis
Copy link
Copy Markdown
Contributor

So to understand the change, you are no longer trying to round the actual time to ns, except for display purposes. The onTimechange is now called with the raw slider value and it wasn't before.

@meganrm
Copy link
Copy Markdown
Contributor Author

meganrm commented Jul 30, 2020

Well before I was more or less converting everything to micro seconds for the slider and then back again to set time. This is how the slider was set up in the example app

const handleTimeChange = (sliderValue: number | [number, number]): void => {
const time = convertSliderValueToNs(sliderValue as number);
onTimeChange(time);
onTimeChange(sliderValue as number); // slider can be a list of numbers, but we're just using a single value
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I don't quite understand this comment; is it saying something about the behavior of the slider input element or something different?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

the type definition for sliderValue in the handleTimeChange function signature allows either number or array of 2 numbers, but onTimeChange only allows a number ?

@meganrm meganrm merged commit d430c22 into master Jul 30, 2020
@meganrm meganrm deleted the fix/time-drag-n-drop branch July 30, 2020 18:41
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.

2 participants