DH Code review

[rlskoeser]

This project was soft-launched in the spring; production site is accessible at https://lenapetimetree.indigenous.princeton.edu/

The readme includes instructions on setting it up to run locally, if desired or helpful for review. Functionality included in the 1.0 release is documented in the change log.

This PR includes all of the JavaScript code (and only that). It's probably more than should be reviewed. Unit tests were added to tests specific parts of the code where it was helpful for development; my goal in this project was not complete coverage.

The code uses a d3.js force layout with custom forces to generate a tree structure with data points grouped into five ordered branches and roughly ordered by date. Interaction logic for selecting leaves in the tree, fetching and displaying contents, selecting tags and highlighting leaves is implemented with d3 and JavaScript (no other framework). The main entry point for the code is timetree.js.

This is written with d3.js v7.8.4; javascript dependencies are installed with npm and imported into local js code; javascript is compiled and embedded using Hugo pipelines.

The JSON data used for the timetree visualization is embedded in the page, but the structure and content is viewable at this url: https://lenapetimetree.indigenous.princeton.edu/index.json

If you have questions about the Hugo template logic used to generate the JSON, or any other parts of the application that aren't included in this review, please let me know and I can point you to the relevant parts of the code.

Feedback I'm interested in at this point:

• suggestions for better JS code organization; the project grew as I was working on it and is very interconnected; I tried to make more modular but not sure how successfully
• suggestions for making it more efficient, especially when first loading the data and laying out the tree
• ideas for adjusting the layout so leaves will be positioned closer to y-coordinate for their date
• any other recommendations for improvements, e.g. to make it more efficient, more idiomatic JavaScript, better documented; notes on what else should be tested
• recommendations for how to split out code/logic from content (everything is currently in one repository); should it just be a hugo theme + hugo site? Would it make any sense (and have any value) for the timetree js code to be a separate javascript library?

[rlskoeser]

@ColeDCrawford here are the more d3-specific parts of the code that I think you should focus on. All of these are
in the timetree.js file: generateNetwork, drawTimeTree; methods for custom forces on the network: leafBounds, centuryY, branchX; updatePositions. If time allows, you could also review the zoom logic, particularly zoomToDatum, zoomToSelectedLeaf, zoomToTagged

If you want to run locally and inspect the network, set visual_debug: true in the hugo config file config.yaml

[rlskoeser]

@raffazizzi I think it would make sense for you to use panel.js as the main entry point and then look at related portions of Leaf class logic in leaves.js. A panel object is initialized in the timetree constructor and an instance is passed into the leaf object instance. In the Leaf class, getCurrentStateis the method where I'm using the url hash and parameters to track selected leaf and tag; other relevant methods are handleClosePanel, currentLeaf and currentTag setters, and updateSelection.

[raffazizzi]

I took a look on the overall structure of the app, and focused more carefully on panel.js and the Leaf class in leaves.js. I also read through the code for the d3 viz (particularly timetree.js), which seems clear enough to me, but I didn't spend as much time on it.

I think the overall structure is clear, and likely keeps the JS to a bare minimum (which can be desirable!). Though, and I think I suggested this during the initial call, if this application where to be generalized to be re-used with other data in other projects, it may be useful looking at a framework to facilitate certain operations. For example, to better handle events (e.g. in panel.js and leaf.js, but also in the class in keys.js, a sort of App-level component), or even to avoid event handling in favor of state management. panel.js and the Leaf class in leaves.js could both be though of as "components" and could be expressed as reusable web components, or framework-specific components (React, Vue...). The need for fetching, parsing, and embedding HTML data in panel.js is a good indication that you probably want to use component composition. See the review comments for more details.

A less important comment: I find the navigation at the root of the tree hard to identify as... navigation. The terms seemed like nodes relevant to the visualization and it took me a while to realize they're clickable. I would make that more apparent, e.g. with a mouseover highlight, or just by making them more typically recognizable as a navbar.

[raffazizzi]

I think this function should only return a string (currently it returns either a string or void). It may be safer to throw an error when branchSlug is undefined. If throwing an error is inconvenient, I would return an empty string and report on the console with either console.log() or console.error())

[raffazizzi]

Elsewhere you used the more concise [] (e.g. in line 171)

[raffazizzi]

This seems to reset previousBranchIndex to 0 at every new branch -- does it need trunkNodeIndex at all?

[raffazizzi]

unused?

[rlskoeser]

yes, duplicate - thanks for catching

[raffazizzi]

unused?

[rlskoeser]

It sure is. Thanks

[raffazizzi]

generally, it's good practice to remove event listeners when they are no longer needed to avoid memory leaks and a progressive slow down of the application. A lot of the addEventListeners here are attached to the container (the panel, I believe) so they're not too risky: they are attached when the page loads and are needed active until the user navigates away or refreshes the page.
I wonder if others could be a bit more leaky, however.

[raffazizzi]

This may be an event worth turning off for tags that are no longer active. If a user clicks on the same tag after removing it, a new eventListener will be created and the previous one will still be in memory!
I think it should be possible to do so here, but you won't be able to use an anonymous function. Here is a guess (not tested):

if (this.activeTagClose) {
  const handleTagDeselect = (event) => {
    this.currentTag = null;
    this.container.dispatchEvent(TagDeselectEvent);
    this.activeTagClose.removeEventListener("click", handleTagDeselect)
  };
  this.activeTagClose.addEventListener("click", handleTagDeselect)
}

[rlskoeser]

I had to look back at the template code - the active tag close button is always there, it's just not visible when there is no active tag. So this shouldn't ever create any new event listeners, and it's only bound once when the page is loaded. But I did notice I have a delegated click handler just above that and wonder if it would make sense to consolidate them....

[raffazizzi]

I think that's already the case, but I would limit d3 DOM operations to SVG nodes in the visualization.

[raffazizzi]

This is fun :)

[raffazizzi]

I have done this in another project as well where I wanted to bypass the animation. So I think this is the way to go. I had many more nodes so the loading time was longer, however, and without the animation it's important to give the user some feedback. It seems like this isn't a problem for the current size of the tree, but this is a place to keep an eye on if the data will grow.

[ColeDCrawford]

Is this the only class that uses the TimeTreeKeysMixin?

[ColeDCrawford]

More of a question - interested why you chose to inject the data this way rather than through an embedded script or reading a CSV / JSON blob with D3. Had a similar problem come up recently and went back and forth on different approaches. The way it's rendered isn't very readable so that doesn't seem to be an advantage.

[rlskoeser]

Thanks for flagging; I don't remember why I did it this way! I agree that using a script tag with type="application/json" is better. Just figured out how to generate that with Hugo (had to figure out how to tell it not to escape my json 😀 ) and updated the parse calls.

[ColeDCrawford]

If you're trying to reuse this in different contexts, maybe pull this out into config.

[ColeDCrawford]

In general the keyboard / voiceover nav works really well. It took a little bit for me to understand why tabbing through leaves was not in visual order (because the data is in chronological order) but I don't think it's a major issue. Kind of fits with the aesthetic and purpose of the vis which is not necessarily to find specific data but to browse. Much more accessible than many D3 visualizations.