-
Notifications
You must be signed in to change notification settings - Fork 61
font size slider for visualizer #256
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
base: unstable
Are you sure you want to change the base?
Conversation
WalkthroughDynamic layout scaling for network graphs, a new Font Size control in the visualization UI, per-node font-size wiring in the client script, edge thickness switched to width-based values, and a new unit test for commodity graph styling. Changes
Sequence Diagram(s)(Skipped — changes are dispersed across UI, layout logic, styling, and tests but do not introduce a single new multi-component sequential flow requiring diagramming.) Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
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.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@temoa/utilities/graph_utils.py`:
- Around line 234-241: The iterator all_edges is being consumed by num_edges =
sum(1 for _ in all_edges) causing the later loop for edge_tuple in all_edges to
see no items; instead materialize the iterable at the start (e.g., convert
all_edges to a list) before counting so num_edges and the subsequent loop both
operate on the same concrete sequence—update references around num_edges,
layout_radius, jitter_radius, and the for edge_tuple in all_edges loop to use
the materialized collection.
In `@temoa/utilities/network_vis_templates/graph_script.js`:
- Around line 35-42: The code attaches internal state to a global by assigning
window.__graph = { data, allNodesPrimary, allEdgesPrimary, allNodesSecondary,
allEdgesSecondary, optionsObject };; either remove this assignment before
production or gate it behind a clear flag/environment check and add a short
comment describing its debugging purpose. Update the block around the
window.__graph assignment (the symbol window.__graph and the variables data,
allNodesPrimary, allEdgesPrimary, allNodesSecondary, allEdgesSecondary,
optionsObject) to only export when a DEBUG/DEV_MODE flag is true or when
process.env.NODE_ENV !== 'production', and include a one-line comment stating
“Expose for debugging only — disable in production.”
In `@temoa/utilities/network_vis_templates/graph_template.html`:
- Around line 20-26: Summary: tighten the font-size slider range to a more
practical maximum. Update the input element with id "font-size-slider" to reduce
the max from 100 to 30 (and optionally add step="1" for finer control) while
keeping value="14" to match DEFAULT_VIS_OPTIONS; ensure any JS that reads the
slider (e.g., handlers referencing "font-size-slider") still accepts the new
range.
In `@tests/test_commodity_visualizer.py`:
- Around line 16-23: Replace the MagicMock usage in
tests/test_commodity_visualizer.py with a concrete NetworkModelData instance so
attribute lookups behave like the real class; instantiate NetworkModelData(),
then set network_data.physical_commodities = {'comm_inter'}, assign
network_data.source_commodities[(region, period)] = {'comm_source'} and
network_data.demand_commodities[(region, period)] = {'comm_demand'} and ensure
network_data.available_techs and network_data.tech_data use the class defaults
(or populate them minimally) before passing network_data into the
CommodityVisualizer test.
| <div class="legend-section"> | ||
| <h4>Visual Settings</h4> | ||
| <div class="control-group"> | ||
| <label for="font-size-slider">Label Font Size</label> | ||
| <input type="range" id="font-size-slider" min="6" max="100" value="14"> | ||
| </div> | ||
| </div> |
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.
🧹 Nitpick | 🔵 Trivial
LGTM! Font size slider integration.
The slider is well-integrated with proper labeling and accessibility attributes. The default value of 14 correctly matches DEFAULT_VIS_OPTIONS.
One consideration: a max of 100 is quite large for label fonts. If most users will stay within 6-30, you could consider tightening the range for finer control, but the current range does allow for extreme zoom-out scenarios where larger labels might be useful.
🤖 Prompt for AI Agents
In `@temoa/utilities/network_vis_templates/graph_template.html` around lines 20 -
26, Summary: tighten the font-size slider range to a more practical maximum.
Update the input element with id "font-size-slider" to reduce the max from 100
to 30 (and optionally add step="1" for finer control) while keeping value="14"
to match DEFAULT_VIS_OPTIONS; ensure any JS that reads the slider (e.g.,
handlers referencing "font-size-slider") still accepts the new range.
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.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@temoa/utilities/network_vis_templates/graph_script.js`:
- Around line 50-52: visualState.fontSize is hardcoded to 14 while optionsObject
(from options_json_str) may specify a different font size, causing a UI/render
mismatch; update the initialization so visualState.fontSize =
(optionsObject?.font_size ?? 14) and set the slider's initial value to that same
value during the UI setup (use the same slider variable or DOM element used
where the slider is created) so both the rendered network and the displayed
slider are synchronized; locate references to visualState, optionsObject, and
the slider initialization code (where the slider value is set) and replace the
hardcoded 14 with the derived value and apply it to the slider control.
| let visualState = { | ||
| fontSize: 14 | ||
| }; |
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.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find and read the graph_script.js file
find . -name "graph_script.js" -type fRepository: TemoaProject/temoa
Length of output: 162
🏁 Script executed:
#!/bin/bash
# Read the graph_script.js file to understand the full context
wc -l ./temoa/utilities/network_vis_templates/graph_script.jsRepository: TemoaProject/temoa
Length of output: 121
🏁 Script executed:
#!/bin/bash
# Read the relevant sections of the file
cat -n ./temoa/utilities/network_vis_templates/graph_script.js | head -120 | tail -80Repository: TemoaProject/temoa
Length of output: 3994
🏁 Script executed:
#!/bin/bash
# Read the beginning of the file to understand how optionsObject is created
cat -n ./temoa/utilities/network_vis_templates/graph_script.js | head -50Repository: TemoaProject/temoa
Length of output: 2122
🏁 Script executed:
#!/bin/bash
# Search for font size references and how optionsObject is used
rg -n "fontSize|font.*size" ./temoa/utilities/network_vis_templates/graph_script.jsRepository: TemoaProject/temoa
Length of output: 709
🏁 Script executed:
#!/bin/bash
# Search for optionsObject usage and structure to see if it contains font settings
rg -n "optionsObject" ./temoa/utilities/network_vis_templates/graph_script.js | head -20Repository: TemoaProject/temoa
Length of output: 350
🏁 Script executed:
#!/bin/bash
# Look for where optionsRaw/options_json_str is generated, likely in a Python backend
find . -name "*.py" -type f | xargs rg -l "options_json_str|network_vis" | head -10Repository: TemoaProject/temoa
Length of output: 93
🏁 Script executed:
#!/bin/bash
# Read the visualizer.py file to understand how options_json_str is generated
wc -l ./temoa/utilities/visualizer.pyRepository: TemoaProject/temoa
Length of output: 97
🏁 Script executed:
#!/bin/bash
# Read the visualizer.py file to find options_json_str generation
rg -n "options_json_str|nodes.*font|edges.*font" ./temoa/utilities/visualizer.pyRepository: TemoaProject/temoa
Length of output: 110
🏁 Script executed:
#!/bin/bash
# Read the visualizer.py file around line 242 and look for options construction
cat -n ./temoa/utilities/visualizer.py | sed -n '200,280p'Repository: TemoaProject/temoa
Length of output: 3792
🏁 Script executed:
#!/bin/bash
# Look at the DEFAULT_VIS_OPTIONS to understand the structure
cat -n ./temoa/utilities/visualizer.py | sed -n '272,315p'Repository: TemoaProject/temoa
Length of output: 1672
🏁 Script executed:
#!/bin/bash
# Check if the slider has a default value in the HTML template
rg -n "font-size-slider" ./temoa/utilities/network_vis_templates/ -A 2 -B 2Repository: TemoaProject/temoa
Length of output: 1411
🏁 Script executed:
#!/bin/bash
# Verify scope - check that optionsObject is defined before line 50
cat -n ./temoa/utilities/network_vis_templates/graph_script.js | sed -n '30,55p'Repository: TemoaProject/temoa
Length of output: 1021
Synchronize slider initial value with backend font size defaults to prevent UI/render mismatch.
visualState.fontSize and the slider are hardcoded to 14, but optionsObject (derived from backend options_json_str) may specify a different font size. When the network initializes, it uses the size from optionsObject, creating a mismatch where the actual render differs from the displayed slider value. Initialize both from optionsObject with a fallback to 14, and set the slider's initial value accordingly.
🛠️ Suggested fix
- let visualState = {
- fontSize: 14
- };
+ const parsedFontSize = parseInt(
+ optionsObject?.nodes?.font?.size ?? optionsObject?.edges?.font?.size ?? 14,
+ 10,
+ );
+ const defaultFontSize = Number.isFinite(parsedFontSize) ? parsedFontSize : 14;
+ let visualState = {
+ fontSize: defaultFontSize
+ };And at line 60:
const fontSizeSlider = document.getElementById('font-size-slider');
+ if (fontSizeSlider) fontSizeSlider.value = String(defaultFontSize);🤖 Prompt for AI Agents
In `@temoa/utilities/network_vis_templates/graph_script.js` around lines 50 - 52,
visualState.fontSize is hardcoded to 14 while optionsObject (from
options_json_str) may specify a different font size, causing a UI/render
mismatch; update the initialization so visualState.fontSize =
(optionsObject?.font_size ?? 14) and set the slider's initial value to that same
value during the UI setup (use the same slider variable or DOM element used
where the slider is created) so both the rendered network and the displayed
slider are synchronized; locate references to visualState, optionsObject, and
the slider initialization code (where the slider value is set) and replace the
hardcoded 14 with the derived value and apply it to the slider control.
Added slider for label font size to the visualizer, added tests for special items in legends, updated visualizer embed in the docs
Summary by CodeRabbit
New Features
Improvements
Tests
✏️ Tip: You can customize this high-level summary in your review settings.