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

Implement default folding behaviour of ShapeViewer and ability to unfold #301

Merged

Conversation

JaapRood
Copy link
Contributor

@JaapRood JaapRood commented Aug 6, 2020

This PR implements default folding of the interaction when first rendered in the ShapeViewer.

Rather processing the entire interaction body all at once, we're only rendering rows that will actually be visible. To reduce the amount of visible rows for first render, we only have the first element of every array unfolded, with the rest hidden. When clicking on a folded line, the view model can be updated by only rendering the bit of missing shape and inserting it in the right place, rather than having to recalculate the entire thing.

The exact behaviour of what and what not to fold is arbitrary, but what we have now is a good default until we start considering diffs (out side the scope of this PR). In case of the example I'm testing with, instead of processing and rendering 781 rows straight from the beginning, we collapsed 19 trails and only render 96 rows initially.

Since we start with a small amount of initial rows and can then incrementally process additional subsets, that also gives us the partitioning we need to decide what to perform additional transformations on, like annotating each row with the expected type.

Another thing that's worth mentioning is that the view model expressed using the reducer pattern. This makes all the state transformations into stateless functions, which doesn't only help us get the logic right in this first implementation, but is also a first great step to it being extracted to it's own module later (for use of Typescript, easier testing, re-use of logic, etc). Any additional enhancements are easily expressed as actions as well.

Final note: this is still behind a disabled-by-default feature flag + opt-in URL.

@JaapRood JaapRood force-pushed the feature/shape-viewer-collapsing-trails branch from 858e465 to b252bdb Compare August 7, 2020 13:53
@JaapRood JaapRood marked this pull request as ready for review August 7, 2020 14:05
@JaapRood JaapRood force-pushed the feature/shape-viewer-collapsing-trails branch from c1cb2c3 to 3031193 Compare August 7, 2020 14:08
@JaapRood JaapRood changed the title Implement transformation of rows to remove collapsed rows per arbitrary trail Implement default folding behaviour of ShapeViewer and ability to unfold Aug 7, 2020
@JaapRood JaapRood requested a review from acunniffe August 7, 2020 14:26
@JaapRood
Copy link
Contributor Author

JaapRood commented Aug 7, 2020

@acunniffe see the PR description for what the interesting stuff is to review. I'm very happy with the approach, so it would be good to know if you can follow what's going on before we build on this approach further.

@acunniffe
Copy link
Member

@JaapRood this looks great.

Rather processing the entire interaction body all at once, we're only rendering rows that will actually be visible. To reduce the amount of visible rows for first render, we only have the first element of every array unfolded, with the rest hidden. When clicking on a folded line, the view model can be updated by only rendering the bit of missing shape and inserting it in the right place, rather than having to recalculate the entire thing.

I definitely expected it to end up with properties like you describe. Thought it would come through via React tree folding -- the view model approach makes way more sense and I like how its implemented.

@acunniffe acunniffe merged commit df29c6c into opticdev:develop Aug 7, 2020
@acunniffe acunniffe deleted the feature/shape-viewer-collapsing-trails branch August 7, 2020 18:24
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