Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds support for dynamic, custom info cards in circular graph visualizations. It introduces a new "custom" visualization mode that allows displaying arbitrary dictionary data for each project node, alongside the existing "classic" (single numeric values) and "distribution" (statistical data) modes.
Key Changes
- Added a new
kind="custom"option that accepts dictionary values with consistent keys across all projects - Implemented dynamic card width calculation based on content size for better visual presentation
- Centralized text positioning using
text-anchor="middle"for improved alignment
Reviewed changes
Copilot reviewed 2 out of 3 changed files in this pull request and generated 15 comments.
| File | Description |
|---|---|
| circular_graph/tools/renderer_utils.py | Extended show_info_card() to support custom type with dynamic keys; added show_custom_info_card() function; improved card width calculations in all card types |
| circular_graph/modular_graph.py | Added custom kind validation in __init__; implemented render_custom_content() and generate_custom_info_card() methods; updated existing render methods to pass keys parameter |
| .gitignore | Added demo.ipynb to ignore list |
Comments suppressed due to low confidence (1)
circular_graph/modular_graph.py:193
- Except block directly handles BaseException.
except:
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| }}); | ||
|
|
||
| const card_width = Math.max(base_card_width, maxContentWidth + 40); | ||
| const cardX = x + card_a_x_shift; |
There was a problem hiding this comment.
The cardX variable is defined but never used. This should be removed to improve code clarity.
| const cardX = x + card_a_x_shift; |
| x_shift = x >= limit_width -15 ? (card_width - card_a_x_shift) * -1 : card_a_x_shift; | ||
| const centeredX = x >= limit_width -15 |
There was a problem hiding this comment.
Missing space around operator. Should be x >= limit_width - 15 for consistency with coding standards.
| x_shift = x >= limit_width -15 ? (card_width - card_a_x_shift) * -1 : card_a_x_shift; | |
| const centeredX = x >= limit_width -15 | |
| x_shift = x >= limit_width - 15 ? (card_width - card_a_x_shift) * -1 : card_a_x_shift; | |
| const centeredX = x >= limit_width - 15 |
| projectText.setAttribute("text-anchor", "middle"); | ||
| projectText.setAttribute("y", y + project_text_y_shift * project_text_y_factor); | ||
|
|
||
| sep.setAttribute("x1", x + x_shift +5); |
There was a problem hiding this comment.
Missing space around operator. Should be x + x_shift + 5 for consistency with coding standards.
| sep.setAttribute("x1", x + x_shift +5); | |
| sep.setAttribute("x1", x + x_shift + 5); |
| tspan.textContent = key.charAt(0).toUpperCase() + key.slice(1) + " : " + (data[key] !== undefined ? data[key] : "N/A"); | ||
| tspan.setAttribute("x", cardCenterX); | ||
| tspan.setAttribute("text-anchor", "middle"); | ||
| tspan.setAttribute("y", y + (text_y_shift* text_y_factor) + text_y_margin * index); |
There was a problem hiding this comment.
Missing space around operator. Should be text_y_shift * text_y_factor for consistency with coding standards.
| tspan.setAttribute("y", y + (text_y_shift* text_y_factor) + text_y_margin * index); | |
| tspan.setAttribute("y", y + (text_y_shift * text_y_factor) + text_y_margin * index); |
| except: | ||
| self.max_value = 0 | ||
| elif self.kind == "custom": | ||
| # Validate that all values are dictionaries and have the same keys |
There was a problem hiding this comment.
Potential bug: If data is empty, next(iter(data.values())) will raise a StopIteration exception. Consider adding a check for empty data before this line, e.g., if not data: raise ValueError("data cannot be empty for kind='custom'")
| # Validate that all values are dictionaries and have the same keys | |
| # Validate that all values are dictionaries and have the same keys | |
| if not data: | |
| raise ValueError("data cannot be empty for kind='custom'") |
| ) | ||
| self.keys = list(first_val.keys()) | ||
| for key, val in data.items(): | ||
| if not isinstance(val, dict) or list(val.keys()) != self.keys: |
There was a problem hiding this comment.
The comparison list(val.keys()) != self.keys is order-sensitive. If dictionaries have the same keys but in different order, this will incorrectly fail. Consider using set(val.keys()) != set(self.keys) or sorted(val.keys()) != sorted(self.keys) for order-independent comparison.
| if not isinstance(val, dict) or list(val.keys()) != self.keys: | |
| if not isinstance(val, dict) or set(val.keys()) != set(self.keys): |
| try: | ||
| first_key = self.keys[0] | ||
| self.max_value = max(v[first_key] for v in data.values()) | ||
| except: |
There was a problem hiding this comment.
Bare except clause catches all exceptions including system-exiting exceptions like KeyboardInterrupt and SystemExit. Consider catching specific exceptions (e.g., except (ValueError, KeyError, TypeError)) to allow system exceptions to propagate properly.
| except: | |
| except (KeyError, TypeError, ValueError): |
|
|
||
| return f""" | ||
| (function showInfoCard(el) {{ | ||
| el.style.cursor= "pointer"; |
There was a problem hiding this comment.
Missing space around the = operator. Should be el.style.cursor = "pointer"; for consistency with JavaScript coding standards.
| el.style.cursor= "pointer"; | |
| el.style.cursor = "pointer"; |
| const centeredX = x >= limit_width -15 | ||
| ? cardX - (card_width - projectTextWidth) | ||
| : cardX + (card_width - projectTextWidth) / 2; |
There was a problem hiding this comment.
The centeredX variable is defined but never used. The code now uses cardCenterX (line 337) for centering text. This variable declaration should be removed to avoid confusion and improve code maintainability.
| const centeredX = x >= limit_width -15 | |
| ? cardX - (card_width - projectTextWidth) | |
| : cardX + (card_width - projectTextWidth) / 2; |
| """ | ||
|
|
||
| if object_attrs is None: | ||
| object_attrs = {} |
There was a problem hiding this comment.
Variable object_attrs is not used.
No description provided.