Refactor header styles and improve responsiveness for better UX - #108
Conversation
…t-for-better-ux Compact header height for better above-the-fold density
…t-for-better-ux Header/share UI refactor: add header status badge, tweak header styles and responsiveness
…t-for-better-ux-d99l1w UI: show read-only header badge for shared links and refine header layout/responsive styles
Reduced header height and adjusted badge styles for better UX.
…t-for-better-ux-cmpi18 Show read-only status via header badge and refactor header styles/responsiveness
…t-for-better-ux-jmpkiv Show read-only badge for shared links and refine header/responsive styles
…t-for-better-ux-sokajx Show read-only header badge for shared links and refine header/layout styles
…t-for-better-ux-l3icpc Show header status badge for shared (read-only) views and refine header responsiveness/styles
There was a problem hiding this comment.
Code Review
This pull request refactors the header layout and styling, introducing a new status badge system for read-only mode and improving responsive behavior. Feedback highlights a critical issue with duplicate HTML IDs for the status badge in the HTML template, which prevents correct DOM updates, and suggests using a class-based selector to update all badge instances simultaneously.
| </div> | ||
| </div> | ||
| <div class="data-time-box"> | ||
| <span class="header-status-badge" id="headerStatusBadge" aria-live="polite"></span> |
There was a problem hiding this comment.
The ID headerStatusBadge is duplicated here; it was already defined on line 75. HTML IDs must be unique within a document. This duplication will cause document.getElementById('headerStatusBadge') in share.js to only return the first occurrence (the one in the logo area), leaving this second badge unpopulated. Consider using a class for these badges or ensuring each ID is unique.
| const headerStatusBadge = document.getElementById('headerStatusBadge'); | ||
| if (headerStatusBadge) headerStatusBadge.textContent = '僅供檢視'; |
There was a problem hiding this comment.
Since there are multiple elements with the class header-status-badge in the HTML (and currently duplicate IDs), using getElementById will only update the first instance found. To ensure all status badges are updated correctly across different responsive layouts, it is better to use querySelectorAll with the class name.
| const headerStatusBadge = document.getElementById('headerStatusBadge'); | |
| if (headerStatusBadge) headerStatusBadge.textContent = '僅供檢視'; | |
| document.querySelectorAll('.header-status-badge').forEach(el => { | |
| el.textContent = '僅供檢視'; | |
| }); |
There was a problem hiding this comment.
Pull request overview
This PR refactors the header layout/styles and adjusts responsive behavior to improve usability on smaller screens, including a new read-only status indicator shown when viewing shared links.
Changes:
- Update header markup to group title/subtitle and introduce a “status badge” element for read-only mode.
- Refine header/responsive CSS (spacing, wrapping behavior, smaller controls on mobile).
- Update share-link flow to set the new header status badge text in read-only mode.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| public/index.html | Updates header DOM structure; adds status badge element; tweaks button accessibility labels and build asset hashes. |
| frontend/styles/header.css | Refactors header layout styles and introduces .header-status-badge styling/visibility rules. |
| frontend/styles/responsive.css | Adjusts mobile header wrapping and button text visibility at small widths. |
| frontend/styles/utilities.css | Shrinks theme toggle button dimensions; removes now-unused .readonly-badge styling. |
| frontend/styles/compatibility.css | Updates read-only-mode header/time box layout rules. |
| frontend/share.js | Switches read-only indicator logic to populate the new header status badge. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| </div> | ||
| </div> | ||
| <div class="data-time-box"> | ||
| <span class="header-status-badge" id="headerStatusBadge" aria-live="polite"></span> |
There was a problem hiding this comment.
id="headerStatusBadge" is used twice in the same document (once in .subtitle-row and again in .data-time-box). Duplicate IDs are invalid HTML and will cause getElementById('headerStatusBadge') to only target one of them, leaving the other badge out of sync. Make the ID unique (or remove the ID and target by class, updating all matching badges).
| <span class="header-status-badge" id="headerStatusBadge" aria-live="polite"></span> | |
| <span class="header-status-badge" aria-live="polite"></span> |
| const headerStatusBadge = document.getElementById('headerStatusBadge'); | ||
| if (headerStatusBadge) headerStatusBadge.textContent = '僅供檢視'; |
There was a problem hiding this comment.
The page now contains multiple .header-status-badge elements, but this code uses getElementById('headerStatusBadge'), which will only ever update a single element (and is especially problematic given the current duplicate IDs in the HTML). Either ensure there is exactly one badge element in the DOM, or update all badges via a class selector (e.g., querySelectorAll('.header-status-badge')).
| const headerStatusBadge = document.getElementById('headerStatusBadge'); | |
| if (headerStatusBadge) headerStatusBadge.textContent = '僅供檢視'; | |
| const headerStatusBadges = document.querySelectorAll('.header-status-badge'); | |
| headerStatusBadges.forEach(badge => { | |
| badge.textContent = '僅供檢視'; | |
| }); |
…t-for-better-ux-vbfbwp Header & responsive layout polish; add read-only status badge for shared links
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| <p class="subtitle" id="examTitle">--</p> | ||
| <div class="subtitle-row"> | ||
| <p class="subtitle" id="examTitle">--</p> | ||
| <span class="header-status-badge" aria-live="polite"></span> |
There was a problem hiding this comment.
public/index.html now renders two .header-status-badge live regions (one next to the subtitle and another inside .data-time-box). In read-only mode both will become visible and both can announce updates via aria-live, which is confusing (duplicate badge text / double announcements). Consider keeping a single status badge element (or hide one via responsive CSS and ensure only one has aria-live).
| <span class="header-status-badge" aria-live="polite"></span> | |
| <span class="header-status-badge" aria-hidden="true"></span> |
| document.querySelectorAll('.header-status-badge') | ||
| .forEach((headerStatusBadge) => { | ||
| headerStatusBadge.textContent = '僅供檢視'; | ||
| }); |
There was a problem hiding this comment.
This updates all .header-status-badge nodes. Given the header now includes multiple badges, this will duplicate the read-only status text and may trigger multiple aria-live announcements. Prefer targeting a single, well-defined element (e.g., #headerStatusBadge) or ensure only one badge exists. Also, since the old DOM-inserted .share-indicator was removed, the remaining .share-indicator CSS in frontend/styles/compatibility.css appears unused and can be removed to avoid dead styles.
| document.querySelectorAll('.header-status-badge') | |
| .forEach((headerStatusBadge) => { | |
| headerStatusBadge.textContent = '僅供檢視'; | |
| }); | |
| const headerStatusBadge = document.getElementById('headerStatusBadge'); | |
| if (headerStatusBadge) { | |
| headerStatusBadge.textContent = '僅供檢視'; | |
| } |
| .time-label { | ||
| display: none; | ||
|
|
||
| } |
There was a problem hiding this comment.
.time-label is now display: none unconditionally, which removes the visible (and screen-reader) context for what #updateTime represents. If the label should be visually hidden for layout reasons, consider keeping it accessible (e.g., via a visually-hidden utility) or adding an aria-label/aria-labelledby relationship so the update time still has a meaningful accessible name.
…t-for-better-ux-w2l7ar Improve header layout and read-only share UI with status badge, responsive tweaks, and accessibility updates
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| } | ||
|
|
||
| .time-label { | ||
| display: none; |
There was a problem hiding this comment.
On small screens, .time-label { display: none; } hides the element that #updateTime references via aria-labelledby="updateTimeLabel". When the referenced label is display:none, many screen readers/browsers won't expose it, which can break the accessible name for the update time. Prefer keeping the label visually-hidden (like the .time-label definition in header.css) rather than display:none.
| display: none; | |
| position: absolute; | |
| width: 1px; | |
| height: 1px; | |
| padding: 0; | |
| margin: -1px; | |
| overflow: hidden; | |
| clip: rect(0, 0, 0, 0); | |
| white-space: nowrap; | |
| border: 0; |
No description provided.