Improve second tree loading & parsing #1788
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jameshadfield
commented
Jun 12, 2024
| const tree1count = this.props.totalStateCounts[filterName]?.get(item.value) ?? 0; | ||
| if (this.props.totalStateCountsSecondTree) { | ||
| const tree2count = this.props.totalStateCountsSecondTree[filterName]?.get(item.value) ?? 0; | ||
| label+=` (L: ${tree1count}, R: ${tree2count})`; |
There was a problem hiding this comment.
Here's an example of how this looks:
jameshadfield
force-pushed
the
james/improve-second-tree-loading
branch
from
dd95486 to
aea6330
Compare
genehack
approved these changes
Jun 13, 2024
jameshadfield
force-pushed
the
james/improve-second-tree-loading
branch
from
aea6330 to
265f8ff
Compare
There are two ways of loading a second tree: upon initial load / narrative slide progression (logic within `createStateFromQueryOrJSONs`) or via the sidebar "second tree" UI (logic within `createTreeTooState`). This commit aims to reduce the duplication of code between these functions, where possible given the current design. There is one behavioural change - the second method of loading a tree now no longer skips the `castIncorrectTypes` error correction function. Tangentially: the code in this file is fragile and the order of function calls is crucial. I hope the adoption of TS will improve resilience here.
Trees define traitNames and traitValues via their `node_attrs` in the
dataset JSON, for instance "traitName=country" and "traitValue=New
Zealand". We populate the filtering dropdowns using these, but had
previously only considered the main (LHS) tree. Behavioural changes:
* traitName & traitValue present on both trees: no change. Filtering
works as expected.
* traitName on both trees, traitValue only on 2nd tree: functionality
added here.
* traitName only present on 2nd tree: this now works, but the app
crashes from an uncaught error in the filter badges shown in the
header. This will be fixed in the next commit.
In addition, mutations and node ("sample") names unique to the 2nd tree
are added to the available filters.
Closes #1781
The previous implementation only reflected counts in the LHS tree, irregardless of any occurrences which may or may not be present in the RHS tree. We now communicate both. This also fixes a bug introduced in the previous commit where filtering to a traitName not present in the LHS tree would cause an uncaught exception.
Branch labels were already rendered correctly on the RHS tree if the key was present in the LHS tree, so all that's required is to add label keys which are only observed in the RHS tree to appear in the sidebar branch label selector. Closes #1780
jameshadfield
force-pushed
the
james/improve-second-tree-loading
branch
from
265f8ff to
99e5336
Compare
|
Rebased onto master (again). Our CI is passing, including our action which builds the docs. Read the docs' own build is failing, but I don't think that's related to changes here. I'll make a separate issue to track this. Merging. |
4 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
See commit messages for more details.