Add Presidential Libraries tracking to American Mapbook - #593
Merged
Conversation
…ng the final count during the ending hold frames instead of only during the animated update frames.
…t-in name and make the tracking selection logic clearer
…lication and simplify future map positioning changes.
…nd Alaska via named constants instead of bare numeric lists
…d of using a None branch, ensuring the applet always uses a valid data set.
…lt-in type name and keep tracking logic naming consistent
…-grouping code easier to understand
…s returned by get_bounds()—changing minx/maxx/miny/maxy lookups to min_x/max_x/min_y/max_y so coordinate scaling works correctly again.
…me instead of permanently, preventing multiple stacked counters and keeping items_to_plot clean
… divide by zero when all points in a group share the same x or y value; in that case, the code now places the result at the midpoint of the target range instead of crashing.
… return the render.Padding object directly
📝 WalkthroughWalkthroughThe American Mapbook now loads U.S. geometry and point datasets from a separate data module, centralizes coordinate conversion, updates animation frame construction, and adds presidential-library tracking configuration. ChangesAmerican Mapbook
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant main
participant get_tracking_items
participant get_presidential_library_options
participant presidential_libraries
main->>get_tracking_items: pass presidential_libraries tracking type
get_tracking_items->>get_presidential_library_options: build library toggle fields
get_presidential_library_options->>presidential_libraries: read library entries
get_presidential_library_options-->>get_tracking_items: return library toggles
get_tracking_items-->>main: return tracking items
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
apps/americanmapbook/americanmapbook.star (2)
245-286: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winDuplicate count-box construction is recomputed unnecessarily 100 times in the tail loop.
The count-box building logic (Lines 245-254 and 273-284) is duplicated verbatim between the per-visited-point loop and the 100-frame "stay on screen" tail loop. In the tail loop,
total_visitedis already final, sorender.Text(...),.size(), and the paddedrender.Boxare being recreated identically on every one of the 100 iterations for no benefit — this is pure wasted work on every render.♻️ Proposed fix — compute once, reuse across the 100 frames
+ final_items = list(items_to_plot) + if config.bool("showCount"): + display_text = render.Text( + content = str(total_visited), + font = font, + color = visited_color, + ) + text_w, text_h = display_text.size() + pad = 2 if is2x else 1 + final_items.append( + add_padding_to_child_element( + render.Box( + color = "`#000`", + width = text_w, + height = text_h, + child = display_text, + ), + width - pad - text_w, + height - pad - text_h, + ), + ) + # Add several frames of the final product to keep on screen for longer for _ in range(100): - final_items = list(items_to_plot) - - if config.get("showCount") == "true": - display_text = render.Text( - content = str(total_visited), - font = font, - color = visited_color, - ) - text_w, text_h = display_text.size() - pad = 2 if is2x else 1 - - final_items.append( - add_padding_to_child_element( - render.Box( - color = "`#000`", - width = text_w, - height = text_h, - child = display_text, - ), - width - pad - text_w, - height - pad - text_h, - ), - ) - animation_frames.append(render.Stack(children = final_items))🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/americanmapbook/americanmapbook.star` around lines 245 - 286, Extract the final count-box construction used in the tail loop into a value computed once before the 100-iteration loop, then append that reused element to each frame when showCount is enabled. Update the tail-loop logic around render.Text, size(), and add_padding_to_child_element so these operations are not repeated, while preserving the existing frame contents and behavior.
196-196: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse
config.bool()instead ofconfig.get(...) == "true"for Toggle fields.All three sites read a
schema.Togglevalue viaconfig.get(key) == "true". Per pixlet's own docs, Toggle values should be read withconfig.bool(), which returns an actual boolean instead of relying on string-literal comparison.♻️ Proposed fix
- if config.get("%s_%s_%s" % (preface, slugify(location["state"]), slugify(location[config_item.lower()]))) == "true": + if config.bool("%s_%s_%s" % (preface, slugify(location["state"]), slugify(location[config_item.lower()]))):- if config.get("showCount") == "true": + if config.bool("showCount"):(apply the same replacement at both Line 237 and Line 264)
As per coding guidelines, "Retrieve boolean options from
schema.Togglewithconfig.bool("key")rather thanconfig.get("key")."Also applies to: 237-237, 264-264
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/americanmapbook/americanmapbook.star` at line 196, Replace the Toggle-value checks in the conditions at the shown locations with config.bool() using the same constructed key, removing the string comparison to "true". Apply this consistently to all three occurrences, including the corresponding checks near the other two locations, while preserving the existing control flow.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/americanmapbook/americanmapbook.star`:
- Around line 122-129: Update the dot_size initialization near the canvas
dimension setup to scale with is2x, using the established 2x rendering
convention so it is 2 on high-density canvases and 1 otherwise. Remove the stale
inline comment and keep the resulting dot_size value shared by both outline
padding and visible dot rendering.
- Around line 172-196: Update get_location_options, get_national_park_options,
and get_world_heritage_options to build Toggle IDs with the shared slugify()
helper instead of plain space replacement, matching the lookup key constructed
in the tracking flow. Preserve get_presidential_library_options’ existing
slugify behavior so punctuation-containing entries resolve to the same
configuration keys.
---
Nitpick comments:
In `@apps/americanmapbook/americanmapbook.star`:
- Around line 245-286: Extract the final count-box construction used in the tail
loop into a value computed once before the 100-iteration loop, then append that
reused element to each frame when showCount is enabled. Update the tail-loop
logic around render.Text, size(), and add_padding_to_child_element so these
operations are not repeated, while preserving the existing frame contents and
behavior.
- Line 196: Replace the Toggle-value checks in the conditions at the shown
locations with config.bool() using the same constructed key, removing the string
comparison to "true". Apply this consistently to all three occurrences,
including the corresponding checks near the other two locations, while
preserving the existing control flow.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: db4d5906-4536-485e-b87a-48e1aeb0fb1b
📒 Files selected for processing (3)
apps/americanmapbook/americanmapbook.starapps/americanmapbook/americanmapbook_data.starapps/americanmapbook/manifest.yaml
rjison
changed the title
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Add new tracking category - Presidential Libraries
Summary by CodeRabbit
New Features
Bug Fixes
Data Updates