Skip to content

reset sim file data between loads, and clear out blank viewer#88

Merged
meganrm merged 16 commits intomasterfrom
fix/black-viewer
Dec 12, 2020
Merged

reset sim file data between loads, and clear out blank viewer#88
meganrm merged 16 commits intomasterfrom
fix/black-viewer

Conversation

@meganrm
Copy link
Copy Markdown
Contributor

@meganrm meganrm commented Dec 11, 2020

Addressing this issue

To test this:

  1. load trajectory, 2. go back to home 3. click "load your data" card. [before: showed old traj, now shows blank viewer]
  2. load trajectory, 2. go back to home, 3. click same trajectory card as you loaded. [before: showed broken view, now reloads trajectory fresh]

To do this I made a clear sim file logic that resets a lot of state. I also had to track when the app gets to the /viewer url and HOW the user got there (from loading their own trajectory or by going to a blank viewer).

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, including any helpful screenshots.
  • 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, schoinh and toloudis December 11, 2020 19:26
Comment thread src/index.tsx Outdated
Comment thread src/components/LocalFileUpload/custom-request-upload.ts
@meganrm meganrm requested a review from toloudis December 11, 2020 22:56
Copy link
Copy Markdown
Contributor

@schoinh schoinh left a comment

Choose a reason for hiding this comment

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

More comments coming, but here is the first batch :D

// used to decide whether to clear out the viewer
to={{
pathname: VIEWER_PATHNAME,
state: { localFile: true },
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.

Oh this is super cool!

Comment thread src/routes/index.tsx Outdated
Comment thread src/routes/index.tsx
Comment thread src/routes/index.tsx
Comment thread src/state/metadata/actions.ts Outdated
Copy link
Copy Markdown
Contributor

@schoinh schoinh left a comment

Choose a reason for hiding this comment

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

Okay I'm done! All the comments in this batch are just extra discussion

Comment thread src/state/metadata/logics.ts
Comment on lines +74 to +78
// plot data is a separate request, clear it out to avoid
// wrong plot data sticking around if the request fails
clearMetaData = receiveMetadata({
plotData: initialState.plotData,
});
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.

Thanks 😁

Comment on lines +69 to +71
export interface ResetAction {
type: string;
}
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 presence of ToggleAction makes me wonder if we should have another generic interface called SimpleAction or something like that that just has { type: string; } as its shape, since it looks like we have a lot of differently named interfaces that are just that. Maybe a todo comment for this if you agree?

Comment thread src/containers/ViewerPanel/index.tsx
Comment thread src/state/metadata/actions.ts Outdated
RequestNetworkFileAction,
RequestLocalFileAction,
ResetSimFileDataAction,
ClearCSimFileDataAction,
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.

Is the C in ClearCSimFileDataAction a typo or does it stand for something? 😅

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.

🤦🏻‍♀️

@meganrm meganrm requested a review from schoinh December 12, 2020 01:18
Copy link
Copy Markdown
Contributor

@schoinh schoinh left a comment

Choose a reason for hiding this comment

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

Looks good, assuming the typo ClearCSimFileDataAction will be fixed

@meganrm meganrm merged commit 0ce06b7 into master Dec 12, 2020
@meganrm meganrm deleted the fix/black-viewer branch December 14, 2020 18:29
@schoinh schoinh mentioned this pull request Jul 1, 2021
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.

3 participants