-
Notifications
You must be signed in to change notification settings - Fork 0
Make card info dynamic #1
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
Conversation
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.
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; |
Copilot
AI
Nov 28, 2025
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.
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 |
Copilot
AI
Nov 28, 2025
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.
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); |
Copilot
AI
Nov 28, 2025
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.
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); |
Copilot
AI
Nov 28, 2025
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.
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 |
Copilot
AI
Nov 28, 2025
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.
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: |
Copilot
AI
Nov 28, 2025
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.
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: |
Copilot
AI
Nov 28, 2025
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.
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"; |
Copilot
AI
Nov 28, 2025
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.
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; |
Copilot
AI
Nov 28, 2025
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.
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 = {} |
Copilot
AI
Nov 28, 2025
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.
Variable object_attrs is not used.
No description provided.