Refactor GoodsTable layout and add readGoodColor utility for dynamic good color retrieval #148
Conversation
|
@YourDeveloperFriend, any idea why the tests are failing? I tried a few things, but I don't want to dig in too much on an unrelated change. |
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the GoodsTable layout to improve visual organization and add dynamic SVG hex headers that match map hex colors. The changes enhance the table's visual clarity by separating white and black column groups and ensuring color consistency with the game map.
- Restructured the CSS grid layout to group white and black columns separately with dedicated headers
- Added dynamic SVG hex headers that retrieve colors from CSS variables to match map hexes
- Implemented visual styling for black columns with dark backgrounds and white text
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| src/client/grid/readGoodColor.ts | New utility function that reads CSS --good-color variables dynamically with DOM manipulation and caching |
| src/client/game/goods_table.tsx | Major refactor of table layout with grouped columns, HeaderHex component, and dynamic color logic |
| src/client/game/goods_table.module.css | Updated CSS with new grid layout, group styling, and black column visual enhancements |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| export function readGoodColor(g: Good): string { | ||
| if (cache.has(g)) return cache.get(g)!; | ||
| if (typeof document === "undefined") return fallback; | ||
| try { | ||
| const el = document.createElement("div"); | ||
| el.className = goodStyle(g); | ||
| el.style.position = "absolute"; | ||
| el.style.visibility = "hidden"; | ||
| el.style.pointerEvents = "none"; | ||
| document.body.appendChild(el); | ||
| const value = getComputedStyle(el).getPropertyValue("--good-color"); | ||
| document.body.removeChild(el); |
There was a problem hiding this comment.
Creating and removing DOM elements for each color read is expensive. Consider using a single persistent hidden element that gets its className updated, or batch multiple color reads to minimize DOM manipulation.
| export function readGoodColor(g: Good): string { | |
| if (cache.has(g)) return cache.get(g)!; | |
| if (typeof document === "undefined") return fallback; | |
| try { | |
| const el = document.createElement("div"); | |
| el.className = goodStyle(g); | |
| el.style.position = "absolute"; | |
| el.style.visibility = "hidden"; | |
| el.style.pointerEvents = "none"; | |
| document.body.appendChild(el); | |
| const value = getComputedStyle(el).getPropertyValue("--good-color"); | |
| document.body.removeChild(el); | |
| // Persistent hidden element for reading good color | |
| let persistentEl: HTMLDivElement | null = null; | |
| function getPersistentEl(): HTMLDivElement { | |
| if (!persistentEl) { | |
| persistentEl = document.createElement("div"); | |
| persistentEl.style.position = "absolute"; | |
| persistentEl.style.visibility = "hidden"; | |
| persistentEl.style.pointerEvents = "none"; | |
| persistentEl.style.height = "0"; | |
| persistentEl.style.width = "0"; | |
| persistentEl.style.overflow = "hidden"; | |
| document.body.appendChild(persistentEl); | |
| } | |
| return persistentEl; | |
| } | |
| export function readGoodColor(g: Good): string { | |
| if (cache.has(g)) return cache.get(g)!; | |
| if (typeof document === "undefined") return fallback; | |
| try { | |
| const el = getPersistentEl(); | |
| el.className = goodStyle(g); | |
| const value = getComputedStyle(el).getPropertyValue("--good-color"); |
| const avail = (availableCities as any[]).find((a) => | ||
| a.onRoll.some((r: any) => r.group === cityGroup && r.onRoll === onRoll), |
There was a problem hiding this comment.
Using any type defeats TypeScript's type safety. Define proper interfaces for availableCities and its elements to ensure type safety and better code maintainability.
| const avail = (availableCities as any[]).find((a) => | |
| a.onRoll.some((r: any) => r.group === cityGroup && r.onRoll === onRoll), | |
| const avail = (availableCities as AvailableCity[]).find((a: AvailableCity) => | |
| a.onRoll.some((r) => r.group === cityGroup && r.onRoll === onRoll), |
| const avail = (availableCities as any[])[letterIndex]; | ||
| const colorVal = avail.color; |
There was a problem hiding this comment.
Using any type and accessing properties without type checking. This should use proper typing and null checks to prevent runtime errors if avail is undefined or doesn't have a color property.
| const avail = (availableCities as any[])[letterIndex]; | |
| const colorVal = avail.color; | |
| const avail = (availableCities as { color?: Good | Good[] }[])[letterIndex]; | |
| const colorVal = avail && "color" in avail ? avail.color : undefined; |
| // determine a primary good color for this onRoll column | ||
| let primaryGood: Good | undefined = undefined; | ||
| // first try to find the actual City on the map with this onRoll/group so color matches the map hex | ||
| const mapCity = grid.cities().find((c) => |
There was a problem hiding this comment.
Isn't this just the city&urbanizedCity above? Why do you need to find it again?
| } | ||
| // final fallback: use the goods currently in the city/urbanized city if present | ||
| if (primaryGood == null && city != null) { | ||
| for (const g of city) { |
There was a problem hiding this comment.
What is this? This is weird. If the city has no color, then you should have a colorless fallback. See Alabama for an example.
| if (primaryGood == null && urbanizedCity != null) { | ||
| for (const g of urbanizedCity) { | ||
| if (g != null) { | ||
| primaryGood = g as Good; |
There was a problem hiding this comment.
Isn't this urbanizedGood, the thing you're doing below?
| const fillColor = primaryGood != null ? readGoodColor(primaryGood) : defaultColor; | ||
| const goodClass = primaryGood != null ? goodStyle(primaryGood) : ""; | ||
|
|
||
| function parseHexOrRgb(color: string): [number, number, number] { |
There was a problem hiding this comment.
Can you move these functions to the module level? That way we can guarantee you're not mixing variables from the function itself.
| @@ -0,0 +1,26 @@ | |||
| import { Good } from "../../engine/state/good"; | |||
There was a problem hiding this comment.
Use the filename read_good_color to match the convention of the project.
|
@YourDeveloperFriend I will get back to this, btw -- I had a lot of work and other distractions the past couple of months and dropped off. But I also think this is a fairly low priority as well, and I may dabble in another issue before coming back to it, if you don't mind. |
2 participants
Closes #143
This pull request significantly improves the layout, accessibility, and visual consistency of the
GoodsTablecomponent, especially for distinguishing between White and Black groups. The changes introduce new CSS for better grouping and styling, refactor the table rendering logic to use these groups, and add a newHeaderHexcomponent for consistent hex-shaped headers with dynamic coloring. Additionally, a utility is added to reliably read good colors from CSS at runtime.UI and Layout Improvements:
GoodsTableto render columns grouped into White and Black sections, each with improved headers and layout, using new CSS classes likegroupsGrid,groupHeader,blackColumn, andblackHeaderfor clearer visual separation and alignment. [1] [2] [3] [4] [5]Visual Consistency and Accessibility:
HeaderHexcomponent to render hex-shaped SVG headers for each column, matching the map's style and dynamically coloring the hex using the correct good color, with automatic text color for contrast.Color Handling:
readGoodColorutility to dynamically read the--good-colorCSS variable for a given good, ensuring that color assignments in the UI always match the CSS source of truth. [1] [2] [3] [4]Code Organization:
goods_table.tsxto include the new color utility and supporting functions for polygon/hex rendering.These changes collectively make the goods table more visually appealing, maintainable, and accessible, while ensuring color consistency with the rest of the application.