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

Abstract values fetching out of mapped vis components #658

Merged
merged 1 commit into from
May 14, 2021
Merged

Conversation

axelboc
Copy link
Contributor

@axelboc axelboc commented May 12, 2021

First, I created a VisBoundary component to hold the ErrorBoundary and Suspense logic of the vis containers. I did not end up moving the DimensionMapper into this component, as keeping the mapper in the containers means I could use VisBoundary in the RawVisContainer and ScalarVisContainer and remove ErrorBoundary and Suspense from Visualizer.

Second, I moved all the data fetching (useDatasetValue(s)) out of the mapped visualizations and inside two new components: ValueFetcher and NxValuesFetcher. I used the render props pattern, as it is much clearer than my previous state-based solution.

Third, I refactored the way we pass formatters to ScalarVis and MatrixVis. Instead of having a guard and an early return in the mapped vis components, I decided to make use of simple type casting.

Fourth, I ran ts-prune to remove unused exports, notably in guards.ts.


What's next

  • Move prefetching to NxValuesFetcher so retrying a cancelled NeXus visualization doesn't lead to a refetching waterfall.
  • [Can wait] Review current solution for complex visualizations (containers, mapped vis, typings, etc.)
  • [Can wait] Review mapped vis components to see if anything else can be abstracted out of them.

@axelboc axelboc force-pushed the vis-container branch 2 times, most recently from 52d5f61 to b4a38b4 Compare May 12, 2021 11:19
src/h5web/guards.ts Show resolved Hide resolved
src/h5web/vis-packs/core/matrix/utils.ts Show resolved Hide resolved
src/h5web/vis-packs/core/scalar/utils.ts Show resolved Hide resolved
src/h5web/vis-packs/nexus/NxValuesFetcher.tsx Show resolved Hide resolved
src/h5web/visualizer/Visualizer.tsx Show resolved Hide resolved
Copy link
Member

@loichuder loichuder left a comment

Choose a reason for hiding this comment

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

I wasn't sold on the concept but as usual, implementation trumps intuition.

I like the fact that one can write a new fetcher and then use the render props to distribute the values around to the children component as it is done by NeXus.

src/h5web/vis-packs/core/matrix/utils.ts Show resolved Hide resolved
@axelboc axelboc merged commit f13b803 into main May 14, 2021
@axelboc axelboc deleted the vis-container branch May 14, 2021 06:56
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