Skip to content

Mark the tile comment with an icon and drop the italic - #1682

Merged
bdraco merged 8 commits into
mainfrom
device-card-comment-icon
Aug 25, 2026
Merged

Mark the tile comment with an icon and drop the italic#1682
bdraco merged 8 commits into
mainfrom
device-card-comment-icon

Conversation

@bdraco

@bdraco bdraco commented Aug 24, 2026

Copy link
Copy Markdown
Member

What does this implement/fix?

#1671 put the device comment on the tile under the filename, but the only cue separating the two lines was italic, at the same size and quiet colour; on a grid of cards the comment read as a second filename. This sets the line as a standard metadata row: a leading comment icon, upright text, same size and colour as before, with the full text in the title tooltip and a localized aria-label so screen readers can tell the note from the path.

While checking it on real cards two layout problems showed up and are fixed here too: wa-icon boxes its glyph in a 1.25em centred host, which indented the text past the icon; and the header was a two-column flex row, so the filename and comment truncated at the status badge's left edge with dead space beneath it. The header is now a three-column grid (checkbox, text, badge) where only the name row shares its line with the badge and the two lines below span the full width.

Related issue or feature (if applicable):

Types of changes

  • Bugfix (non-breaking change which fixes an issue) — bugfix
  • New feature (non-breaking change which adds functionality) — new-feature
  • Enhancement to an existing feature — enhancement
  • Breaking change (fix or feature that would cause existing functionality to not work as expected) — breaking-change
  • Refactor (no behaviour change) — refactor
  • Documentation only — docs
  • Maintenance / chore — maintenance
  • CI / workflow change — ci
  • Dependencies bump — dependencies

Checklist

  • The code change is tested and works locally. (Checked on the live dashboard against a dev backend in both themes, with and without select mode.)
  • pnpm run lint passes.
  • pnpm run test passes.
  • Tests have been added to verify that the new code works (where applicable).

Copilot AI lite review requested due to automatic review settings August 24, 2026 22:56
@bdraco bdraco added the enhancement Improvement to an existing feature label Aug 24, 2026

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

Adds a comment icon to device-card comments so they are distinct from filenames.

Changes:

  • Registers and renders the MDI comment icon.
  • Updates comment layout and truncation styling.
  • Adds test coverage for the icon.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.

File Summary
test/components/device-card-comment.test.ts Verifies the comment icon.
src/components/device-card/styles.ts Styles the icon and comment layout.
src/components/device-card.ts Registers and renders the icon.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Copilot AI review requested due to automatic review settings August 24, 2026 23:01
@bdraco bdraco changed the title Mark the tile comment with an icon so it reads apart from the filename Mark the tile comment with an icon and drop the italic Aug 24, 2026

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

Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.

Copilot AI review requested due to automatic review settings August 24, 2026 23:04
The header was a two-column flex row, so the filename and comment
truncated at the badge's left edge with dead space beneath it. A grid
keeps the name beside the badge and spans the two lines below it.

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

Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.

Copilot AI review requested due to automatic review settings August 24, 2026 23:06

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

Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.

@esphbot

esphbot commented Aug 24, 2026

Copy link
Copy Markdown
Contributor

Previous review — superseded by a newer review below.

@esphbot esphbot 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.

Tip

No blocking issues found — ready to merge.

…dy styles

Merges the grid placement into each element's existing rule block,
drops the flex-only declarations and the stale flex comment, keeps the
comment icon at full quiet colour since it is the distinguishing cue,
and gives the comment paragraph a localized aria-label so a screen
reader can tell it from the filename.
Copilot AI review requested due to automatic review settings August 24, 2026 23:32
@bdraco

bdraco commented Aug 24, 2026

Copy link
Copy Markdown
Member Author

All five handled: the badge tooltip is pinned to the badge cell so it can never auto-place (its host is display: contents anyway, so this is documentation as much as insurance), the grid placement is merged into each element's single rule block with the flex-only declarations and stale comment removed, the icon opacity is gone, the comment paragraph carries a localized aria-label (new dashboard.device_comment key), and the description now covers the header grid rework with the tested box ticked.

@bdraco

bdraco commented Aug 24, 2026

Copy link
Copy Markdown
Member Author

Correction to my last note: wa-tooltip's host is position: absolute (inline-block), not display: contents. Same outcome, an absolutely positioned child never auto-places in the grid, and the CSS comment now says so.

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

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

Comment thread src/components/device-card.ts Outdated
Copilot AI review requested due to automatic review settings August 24, 2026 23:35

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

Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.

Suppressed comments (3)

src/components/device-card.ts:254

  • aria-label is on a <p> with its native paragraph role, which does not reliably accept an author-provided accessible name, so assistive technology may announce only the raw comment and not distinguish it from the filename. Give this metadata row role="note" (the established pattern in src/components/device/config-entry-renderers-shared.ts:151-170) so the localized label is exposed.
                ? html`<p
                    class="device-comment"
                    title=${this.comment}
                    aria-label="${this._localize("dashboard.device_comment")}: ${this.comment}"

src/components/device-card.ts:256

  • This icon is decorative because the parent supplies the localized accessible name, but it is not marked aria-hidden here. wa-icon can therefore be exposed as a separate graphic (and potentially add an unhelpful icon name) before the comment label; mark it hidden like the other decorative icons in this codebase.
                    <wa-icon library="mdi" name="comment-text-outline"></wa-icon>

src/components/device-card/styles.ts:121

  • .device-config is now a grid item spanning the flexible text track and the auto status track, but it keeps the default min-width: auto. Because .truncate sets white-space: nowrap, a long configuration filename can contribute its min-content width and prevent the grid from shrinking, causing the card to overflow instead of ellipsizing; give this row min-width: 0 as the comment row already has.
    .device-config,
    .device-comment {
      grid-column: 2 / -1;

Copilot AI review requested due to automatic review settings August 24, 2026 23:42

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

Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.

@esphbot

esphbot commented Aug 25, 2026

Copy link
Copy Markdown
Contributor

@bdraco: All five handled: the badge tooltip is pinned to the badge cell so it can never auto-place (its host...

Checked each against the tree. Tooltip pinned r1c3. Checkbox, name-wrap, badge each back to one rule block; flex-shrink gone; checkbox comment cites align-items: start. Icon opacity gone. aria-label present with new key.

Also confirmed placement math: config and comment carry a definite column with auto rows, so they land rows 2 and 3. overflow: hidden from .truncate keeps their min-content at zero, so a long path cannot widen the badge track.

@esphbot

esphbot commented Aug 25, 2026

Copy link
Copy Markdown
Contributor

@bdraco: Correction to my last note: wa-tooltip's host is position: absolute (inline-block), not display: con...

Cannot confirm locally — node_modules absent in the review checkout. Moot regardless: display: contents and position: absolute both skip auto-placement, and the pin is correct under either. Comment now states the actual reason.

@esphbot

esphbot commented Aug 25, 2026

Copy link
Copy Markdown
Contributor

PR Review — Mark the tile comment with an icon and drop the italic

All five prior findings are genuinely fixed in the working tree; two non-blocking nits remain, neither in the layout itself.

Verified in the checkout rather than from the diff: the grid geometry is sound. .device-card-header-left is display: contents, so its three children place directly on the header grid; .device-name-wrap is pinned to r1c2, the badge to r1c3, and .device-config / .device-comment carry a definite column (2 / -1) with auto rows, which walks the placement cursor to rows 2 and 3 — no overlap, no implicit column. Both spanning items resolve a zero automatic minimum (.truncate sets overflow: hidden, plus the explicit min-width: 0), so a long path can't inflate the auto badge track. With selectMode off, column 1 has no items and collapses to zero — and because the spacing is margin-right on the checkbox rather than a column gap, there's no phantom indent, exactly as the CSS comment claims.

On the prior round's findings: the tooltip is pinned to r1c3; .device-checkbox, .device-name-wrap and .device-status are each back to a single rule block with the dead flex-shrink: 0 gone and the checkbox comment now citing align-items: start; the icon opacity is gone; role="note" matches the established pattern (config-entry-renderers-shared.ts:153, notice-banner.ts:50) and is asserted alongside the label in the test.

  • Composite aria-label is concatenated with a hardcoded ": ", and the new dashboard.device_comment key duplicates table_col_comment's English verbatim — one ICU-parameterized key covers both (note: render-bits.ts already concatenates this way, so this is a preference, not a violation)
  • archived-devices-dialog.ts:157 still sets its comment rows italic, so the same field now reads differently on two surfaces — follow-up, not this PR
  • The decorative wa-icon carries no aria-hidden, matching every other decorative icon in this component (pencil, text-box-outline); with role="note" + aria-label the accessible name is fixed regardless, so Copilot's suppressed note on this does not apply here
  • Unverified: node_modules is absent in this worktree, so I could not run the suite or independently confirm wa-tooltip's host positioning — the pin is correct under any host display, so it costs nothing either way

✅ Resolved since last review (3)

Previously-flagged issues verified fixed
  • src/components/device-card/styles.ts:112 Placement rules split from appearance rules, leaving dead flex props and a stale comment
  • src/components/device-card/styles.ts:245 opacity: 0.8 dims the one cue the PR relies on
  • src/components/device-card.ts:251 The comment/filename distinction is still visual-only for screen readers

🟢 Suggestions

1. Composite aria-label built by concatenation rather than a parameterized key
src/components/device-card.ts:250-256

The label is assembled in the template as <localized "Comment"> + a hardcoded ": " + the comment text. Two small consequences:

  • Translators can't control the separator. Several locales don't use ": " (French wants a narrow no-break space before the colon, Japanese uses or nothing), and they never see the joiner because it lives in the template.
  • The new dashboard.device_comment key is a bare "Comment" — byte-identical to the dashboard.table_col_comment two lines above it in en.json:234. Two keys with the same English string is extra work in Lokalise for no added meaning.

The repo's established shape for a composite aria-label is a single ICU-parameterized key — dashboard.labels_remove ("Remove {name}", device-labels-editor.ts:295), settings.build_server_peer_remove_aria ("Remove paired sender {label}", build-server-section.ts:227). That form would fold both points into one key:

"device_comment": "Comment: {comment}"
aria-label=${this._localize("dashboard.device_comment", { comment: this.comment })}

Noting the counter-evidence so this doesn't read as a clean convention violation: render-bits.ts:161 and :207 already concatenate "${label}. ${tooltip}" in this same component, so both shapes exist here. Non-blocking either way — the current label is correct English and does close the a11y gap.

aria-label="${this._localize("dashboard.device_comment")}: ${this.comment}"

Checklist

  • No hardcoded secrets or unsafe operations
  • Grid placement covers every header render branch (checkbox absent, badge tooltip, both badge variants)
  • Truncation preserved — spanning items can't inflate the auto badge track
  • Accessible name for the meaning-carrying decoration
  • New user-facing copy added to en.json only — suggestion #1
  • Localized copy follows the repo's parameterized-key pattern — suggestion #1
  • Test coverage for the added behaviour (icon name, role, label)
  • Styles stay consistent and free of dead declarations

Automated review by Kōan (Claude) HEAD=8684678 3 min 51s

@esphbot esphbot 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.

Tip

No blocking issues found — ready to merge.

@bdraco

bdraco commented Aug 25, 2026

Copy link
Copy Markdown
Member Author

Took the parameterized key: dashboard.device_comment is now "Comment: {comment}" and the template passes the comment in, so translators own the separator and the duplicate bare "Comment" string is gone. The archived dialog's italic is a follow-up as noted.

Copilot AI review requested due to automatic review settings August 25, 2026 00:22
@bdraco
bdraco enabled auto-merge (squash) August 25, 2026 00:22
@bdraco
bdraco merged commit 232822e into main Aug 25, 2026
13 checks passed
@bdraco
bdraco deleted the device-card-comment-icon branch August 25, 2026 00:25

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

Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.

@github-actions github-actions Bot locked and limited conversation to collaborators Aug 26, 2026
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

enhancement Improvement to an existing feature

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants