-
Notifications
You must be signed in to change notification settings - Fork 21
feat(forkchoice): add color legend to fork choice viz #338
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: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -56,6 +56,31 @@ | |||||||||||||||||||||||||||
| .value-finalized { color: #4caf50; } | ||||||||||||||||||||||||||||
| .value-validators { color: #e0e0e0; } | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| #legend { | ||||||||||||||||||||||||||||
| display: flex; | ||||||||||||||||||||||||||||
| gap: 12px; | ||||||||||||||||||||||||||||
| margin-left: auto; | ||||||||||||||||||||||||||||
| flex-wrap: wrap; | ||||||||||||||||||||||||||||
| align-items: center; | ||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| .legend-item { | ||||||||||||||||||||||||||||
| display: flex; | ||||||||||||||||||||||||||||
| align-items: center; | ||||||||||||||||||||||||||||
| gap: 6px; | ||||||||||||||||||||||||||||
| font-size: 11px; | ||||||||||||||||||||||||||||
| color: #888; | ||||||||||||||||||||||||||||
| text-transform: uppercase; | ||||||||||||||||||||||||||||
| letter-spacing: 1px; | ||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| .legend-swatch { | ||||||||||||||||||||||||||||
| width: 10px; | ||||||||||||||||||||||||||||
| height: 10px; | ||||||||||||||||||||||||||||
| border-radius: 50%; | ||||||||||||||||||||||||||||
| flex-shrink: 0; | ||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| #chart-container { | ||||||||||||||||||||||||||||
| flex: 1; | ||||||||||||||||||||||||||||
| overflow: auto; | ||||||||||||||||||||||||||||
|
|
@@ -157,6 +182,12 @@ | |||||||||||||||||||||||||||
| <span class="label">Validators</span> | ||||||||||||||||||||||||||||
| <span class="value value-validators" id="validator-count">--</span> | ||||||||||||||||||||||||||||
| </div> | ||||||||||||||||||||||||||||
| <div id="legend" aria-label="Block color legend"> | ||||||||||||||||||||||||||||
| <div class="legend-item"><span class="legend-swatch" style="background: #4caf50;"></span>Finalized</div> | ||||||||||||||||||||||||||||
| <div class="legend-item"><span class="legend-swatch" style="background: #2196f3;"></span>Justified</div> | ||||||||||||||||||||||||||||
| <div class="legend-item"><span class="legend-swatch" style="background: #ffeb3b;"></span>Safe target</div> | ||||||||||||||||||||||||||||
| <div class="legend-item"><span class="legend-swatch" style="background: #ff9800;"></span>Head</div> | ||||||||||||||||||||||||||||
|
Comment on lines
+186
to
+189
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
The swatch colors are hardcoded as inline Prompt To Fix With AIThis is a comment left during a code review.
Path: crates/net/rpc/static/fork_choice.html
Line: 186-189
Comment:
**Inline hex values will silently drift from `COLORS`**
The swatch colors are hardcoded as inline `style` attributes. The file already has `.value-head`, `.value-justified`, and `.value-finalized` CSS classes whose `color` values are kept in sync with the JS `COLORS` object. If `COLORS` is updated, the legend swatches won't change automatically. Consider using a CSS custom property or at minimum a comment like `/* must match COLORS in JS */` to signal the coupling.
How can I resolve this? If you propose a fix, please make it concise. |
||||||||||||||||||||||||||||
| </div> | ||||||||||||||||||||||||||||
|
Comment on lines
+185
to
+190
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
Prompt To Fix With AIThis is a comment left during a code review.
Path: crates/net/rpc/static/fork_choice.html
Line: 185-190
Comment:
**Legend omits the default (grey) node color**
`nodeColor()` returns `COLORS.default` (`#666`) for any block that isn't head, safe-target, justified, or finalized. Those grey nodes will appear in the viz but aren't explained by the legend, leaving users in the same "have to guess" situation the legend is meant to fix. Adding a fifth swatch for the pending/default state would make the legend complete.
```suggestion
<div id="legend" aria-label="Block color legend" role="region">
<div class="legend-item"><span class="legend-swatch" style="background: #4caf50;"></span>Finalized</div>
<div class="legend-item"><span class="legend-swatch" style="background: #2196f3;"></span>Justified</div>
<div class="legend-item"><span class="legend-swatch" style="background: #ffeb3b;"></span>Safe target</div>
<div class="legend-item"><span class="legend-swatch" style="background: #ff9800;"></span>Head</div>
<div class="legend-item"><span class="legend-swatch" style="background: #666;"></span>Default</div>
</div>
```
How can I resolve this? If you propose a fix, please make it concise. |
||||||||||||||||||||||||||||
| </div> | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| <div id="chart-container"> | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
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.
aria-labelineffective without a role on a genericdivScreen readers only announce
aria-labelon elements that have an implicit or explicit ARIA role. A plain<div>has no role, so the label"Block color legend"will be silently ignored by most assistive technology. Addingrole="region"turns this into a named landmark that screen readers will announce.Prompt To Fix With AI