Cleaner UI #71
Cleaner UI #71
Conversation
There was a problem hiding this comment.
Pull request overview
This pull request introduces significant UI enhancements by adding modal help dialogs and collapsible sections to improve user experience and interface clarity. The changes include new JavaScript functionality for modals and section toggles, comprehensive CSS styling, and HTML structural updates.
Key changes:
- Added modal dialog system with help content for location selection, including overlay, animations, and keyboard/click-to-close functionality
- Implemented collapsible "All Planets" and "Houses" sections with persistent state using localStorage
- Updated UI styling including flex layout for labels, modal styles, and toggleable section styles
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 12 comments.
Show a summary per file
| File | Description |
|---|---|
templates/index.html |
Added help button with modal dialog for location guidance, replacing the inline help section |
templates/full_chart.html |
Converted "All Planets" and "Houses" sections to collapsible format with toggle buttons |
static/js/modal.js |
New file implementing modal open/close functionality with escape key and overlay click handlers |
static/js/section-toggle.js |
New file implementing collapsible section logic with localStorage persistence |
static/css/style.css |
Added modal and toggleable section styles, updated label display to flex, removed chart-section border |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
- Added location help modal trigger in modal.js - Improved section toggle functionality with a dedicated update function in section-toggle.js - Updated HTML structure for better accessibility and styling - Added tests for modal and section toggle JavaScript functions
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 7 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # Test for main functions | ||
| self.assertIn('openModal', content) | ||
| self.assertIn('closeModal', content) | ||
| self.assertIn('initializeModals', content) |
There was a problem hiding this comment.
The test assertions check for the presence of string literals like 'openModal', 'closeModal', etc., but these are very generic terms that could appear in comments, variable names, or other contexts. Consider using more specific pattern matching (e.g., checking for function declarations like 'function openModal' or 'function closeModal') to ensure the actual function definitions exist, not just the text strings.
| // Close modal on Escape key (remove old listener first to prevent duplicates) | ||
| document.removeEventListener('keydown', handleEscapeKey); | ||
| document.addEventListener('keydown', handleEscapeKey); |
There was a problem hiding this comment.
The pattern of removing and then immediately re-adding the event listener for Escape key handling on every modal initialization could lead to issues. If initializeModals() is called multiple times (e.g., in a dynamic page context), this pattern may not prevent duplicate listeners as intended since the listener is added back immediately. Consider using a flag to track whether the listener has been added, or better yet, use the 'once' option or ensure initializeModals() is only called once.
| .help-icon-btn { | ||
| background: transparent; | ||
| border: 2px solid var(--border-color); | ||
| border-radius: 50%; | ||
| width: 44px; | ||
| height: 44px; | ||
| min-width: 44px; | ||
| min-height: 44px; | ||
| display: inline-flex; | ||
| align-items: center; | ||
| justify-content: center; | ||
| cursor: pointer; | ||
| color: var(--text-secondary); | ||
| font-size: 24px; /* Ensures the icon remains visually 24px */ | ||
| font-weight: bold; | ||
| font-family: Arial, sans-serif; | ||
| margin-left: 8px; | ||
| padding: 0; | ||
| line-height: 1; |
There was a problem hiding this comment.
The help icon button has a fixed size of 44x44 pixels with a 24px font size. However, the font-size comment states it "ensures the icon remains visually 24px", but this doesn't scale with user font-size preferences (e.g., for users who need larger text). Consider using relative units like 'em' or 'rem' for the button dimensions and font-size to better support accessibility and user preferences.
| } | ||
|
|
||
| .section-content { | ||
| max-height: none; |
There was a problem hiding this comment.
The transition uses 'max-height' animation which requires knowing the content height. Setting 'max-height: none' for the expanded state and then transitioning to 'max-height: 0' won't animate smoothly. This approach typically needs a fixed max-height value (e.g., '5000px') for the expanded state to allow the transition to work properly. Consider using a specific large value or implementing a JavaScript-based height calculation for smoother animations.
| max-height: none; | |
| max-height: 1000px; |
| font-size: 28px; | ||
| cursor: pointer; | ||
| color: var(--text-tertiary); | ||
| padding: 0; | ||
| width: 44px; | ||
| height: 44px; |
There was a problem hiding this comment.
The modal close button has a fixed size of 44x44 pixels with a 28px font size. Similar to the help icon button, these fixed pixel dimensions don't scale with user font-size preferences. Consider using relative units like 'em' or 'rem' to better support accessibility and user text-size preferences.
| font-size: 28px; | |
| cursor: pointer; | |
| color: var(--text-tertiary); | |
| padding: 0; | |
| width: 44px; | |
| height: 44px; | |
| font-size: 1.75rem; | |
| cursor: pointer; | |
| color: var(--text-tertiary); | |
| padding: 0; | |
| width: 2.75rem; | |
| height: 2.75rem; |
| box-shadow: none; | ||
| } | ||
|
|
||
| .dark-mode .section-header:hover { | ||
| background: rgba(102, 126, 234, 0.1); | ||
| border-color: #667eea; | ||
| box-shadow: none; | ||
| } | ||
|
|
There was a problem hiding this comment.
The 'box-shadow: none' property is set and then explicitly set again in the hover state. This is redundant since the base state already has no box-shadow. Consider removing the explicit 'box-shadow: none' from lines 995, 1001, and 1007 to reduce redundancy.
| box-shadow: none; | |
| } | |
| .dark-mode .section-header:hover { | |
| background: rgba(102, 126, 234, 0.1); | |
| border-color: #667eea; | |
| box-shadow: none; | |
| } | |
| } | |
| .dark-mode .section-header:hover { | |
| background: rgba(102, 126, 234, 0.1); | |
| border-color: #667eea; | |
| } |
| # Test for main functions | ||
| self.assertIn('initializeToggleSections', content) | ||
| self.assertIn('restoreSectionStates', content) | ||
| self.assertIn('updateSectionState', content) |
There was a problem hiding this comment.
The test assertions check for the presence of string literals like 'initializeToggleSections', 'restoreSectionStates', etc., but these are very generic terms that could appear in comments, variable names, or other contexts. Consider using more specific pattern matching (e.g., checking for function declarations like 'function initializeToggleSections') to ensure the actual function definitions exist, not just the text strings.
1 participant
This pull request introduces major UI enhancements to the app by adding modal help dialogs, collapsible (toggleable) sections, and associated styling and JavaScript functionality. The primary goals are to improve user experience and interface clarity, particularly for the birth location selection and the display of astrological chart sections.
Key changes include:
Modal Help Dialog Functionality
index.html. The modal provides guidance on using the interactive map. (templates/index.html,static/js/modal.js,static/css/style.css) [1] [2] [3] [4] [5] [6]modal.jsto handle opening, closing (including overlay click and Escape key), and body scroll locking. (static/js/modal.js)style.cssfor overlays, content, headers, close buttons, and animations. (static/css/style.css)Collapsible (Toggleable) Chart Sections
full_chart.htmlto be collapsible, with toggle buttons and persistent state (usinglocalStorage). (templates/full_chart.html,static/js/section-toggle.js) [1] [2] [3] [4] [5]section-toggle.js) to manage section expand/collapse, including restoring the last state on page load. (static/js/section-toggle.js)static/css/style.css)UI/UX Improvements
static/css/style.css,templates/index.html) [1] [2]static/css/style.css)These updates collectively make the app more interactive and user-friendly, especially for new users needing guidance or those navigating large chart sections.