Skip to content

Show read-only status via header badge and refactor header styles/responsiveness - #104

Merged
alvin000009238 merged 2 commits into
devfrom
codex/reduce-header-height-for-better-ux-cmpi18
Mar 31, 2026
Merged

Show read-only status via header badge and refactor header styles/responsiveness#104
alvin000009238 merged 2 commits into
devfrom
codex/reduce-header-height-for-better-ux-cmpi18

Conversation

@alvin000009238

Copy link
Copy Markdown
Owner

Motivation

  • Indicate when a shared link opens the app in read-only mode using an existing header badge element rather than injecting DOM nodes.
  • Improve header layout, spacing and responsive behavior to prevent overflow and better accommodate status badge and compact controls.
  • Harmonize button sizes and visual tokens to make the header more compact and consistent across breakpoints.

Description

  • frontend/share.js now sets the text of an existing #headerStatusBadge element to 僅供檢視 when loading shared data instead of creating a new .share-indicator node.
  • Added a header status badge element to public/index.html (span #headerStatusBadge) and updated several button ARIA labels and markup wrappers for accessibility and layout.
  • Large CSS refactor across frontend/styles/*: tuned header paddings, reduced icon/button sizes, added .header-status-badge styles, made .header-content wrapping/ellipsis-safe, adjusted .data-time-box and read-only mode rules, and improved responsive rules in responsive.css.
  • Minor utility tweaks in utilities.css (theme toggle sizing) and cleanup of obsolete .readonly-badge usage.

Testing

  • Ran a frontend production build with npm run build, which completed and produced updated asset filenames referenced in public/index.html.
  • Ran the linter (npm run lint) against the frontend sources with no errors.
  • Performed a quick local smoke check of the header and share flow in the dev server confirming the read-only badge appears when loading a shared link.

Codex Task

Copilot AI review requested due to automatic review settings March 31, 2026 14:48
@alvin000009238
alvin000009238 merged commit db7b120 into dev Mar 31, 2026
1 check passed

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request refactors the header layout and styling to improve responsiveness and accessibility. Key changes include replacing manual DOM injection for the read-only indicator with a dedicated status badge, adding ARIA labels to buttons, and implementing a more robust flexbox layout with better overflow handling for titles and subtitles. Review feedback identifies a potential layout regression on desktop where a global width rule for the time box in read-only mode could squash the logo, and a concern regarding low text contrast for timestamps due to the combination of muted colors and low opacity.

I am having trouble creating individual review comments. Click here to see my feedback.

frontend/styles/compatibility.css (103-106)

high

The width: 100% rule for .data-time-box in read-only mode is applied globally. On desktop, where the parent .header-content is a nowrap flex container, this will cause the time box to occupy the full width and squash the logo. This rule is already correctly scoped to mobile in responsive.css and should be removed here to maintain the correct layout on larger screens.

body.read-only-mode .data-time-box {
    justify-content: space-between;
}

frontend/styles/header.css (269-271)

medium

Combining var(--color-text-muted) with opacity: 0.78 for the .time-value significantly reduces text contrast. This may make the timestamp difficult to read, especially on different backgrounds, and could violate accessibility standards. It is recommended to remove the opacity to ensure the text remains legible.

    color: var(--color-text-muted);

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

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 updates the frontend header to surface “read-only” status for shared links via an existing header badge element, while refactoring header layout/styling to be more compact and responsive across breakpoints.

Changes:

  • Replace injected “share indicator” DOM with updates to a new persistent #headerStatusBadge element when loading shared links.
  • Refactor header markup and CSS to support a compact title/subtitle/badge row and tighter button sizing.
  • Adjust responsive/read-only CSS rules for the header, data-time box, and action buttons.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
public/index.html Adds #headerStatusBadge in the header and tweaks button ARIA labels/markup; updates built asset hashes.
frontend/share.js Sets #headerStatusBadge text in shared-link (read-only) flow instead of injecting new nodes.
frontend/styles/header.css Refactors header layout (nowrap flex), adds .header-status-badge styling, and tightens spacing/sizing.
frontend/styles/responsive.css Updates header/data-time/action responsiveness, especially at ≤480px.
frontend/styles/compatibility.css Adjusts read-only mode header/data-time rules.
frontend/styles/utilities.css Tweaks theme toggle button sizing and removes obsolete .readonly-badge.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +104 to 107
width: 100%;
justify-content: space-between;
}

Copilot AI Mar 31, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

body.read-only-mode .data-time-box { width: 100%; } is unlikely to have the intended “full-width row” effect because .header-content is now flex-wrap: nowrap (see frontend/styles/header.css). In read-only mode this will instead cause the flexbox algorithm to aggressively shrink the logo/title area to accommodate the 100% width basis, making the header text truncate more than necessary.

Consider either (a) removing the width: 100% rule and letting .data-time-box size to its contents, or (b) pairing this with a read-only override that allows wrapping (e.g. body.read-only-mode .header-content { flex-wrap: wrap; }) and/or using flex-basis: 100% only when wrapping is enabled.

Suggested change
width: 100%;
justify-content: space-between;
}
flex-basis: 100%;
justify-content: space-between;
}
body.read-only-mode .header-content {
flex-wrap: wrap;
}

Copilot uses AI. Check for mistakes.
@alvin000009238
alvin000009238 deleted the codex/reduce-header-height-for-better-ux-cmpi18 branch April 1, 2026 03:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants