-
Notifications
You must be signed in to change notification settings - Fork 18
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
Fetch whole dataset only when autoScale is off (and slices otherwise) #877
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Apart from the loading message logic, which can be moved inside VisBoundary
, I don't see a lot of duplication between the line and spectrum containers. Where there's more duplication now is between the line and heatmap containers. However, I don't see it as a problem, as all the business logic is nicely encapsulated in shared hooks and components.
Comparing the containers, I've actually just noticed that we have duplicate NeXus type guards: assertNumericNxData
and assertNumericSignal
, and assertComplexNxData
and assertComplexSignal
.
The fact that we use useLineConfig
in the containers can indeed feel like we're breaking one of our design decisions, but perhaps this wasn't a great decision in the first place. It makes sense that the user's preferences would eventually affect the fetching of the data somehow. Perhaps relaxing this decision will open the door to some refactoring to reduce duplication/complexity in the mapped vis components?
packages/app/src/vis-packs/core/complex/ComplexLineVisContainer.tsx
Outdated
Show resolved
Hide resolved
5dfb0d1
to
d0e35c3
Compare
packages/app/src/vis-packs/nexus/containers/NxSpectrumContainer.tsx
Outdated
Show resolved
Hide resolved
d0e35c3
to
dbc0881
Compare
dbc0881
to
b950f5d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice!
b950f5d
to
2c28c6b
Compare
/approve |
f3844d5
to
b3e2bb8
Compare
Improvement for #616
Also:
autoScale
Drafting it for discussion as I feel we can do better here:
LineVis
containers that we could refactoruseLineConfig
was always used inMapped
components but now, I need it outside...