Skip to content

Open the editor from the migration dot and badge - #1653

Merged
bdraco merged 4 commits into
mainfrom
migration-dot-click
Aug 15, 2026
Merged

Open the editor from the migration dot and badge#1653
bdraco merged 4 commits into
mainfrom
migration-dot-click

Conversation

@bdraco

@bdraco bdraco commented Aug 15, 2026

Copy link
Copy Markdown
Member

What does this implement/fix?

Clicking the purple migration dot on a device card or table row, or the "Config migration available" badge in the drawer, now opens the editor; the migrate nudge is right there, so the next click is Update config. The indicators fire a dedicated open-config-migration event bound to editDevice(device, { reveal: true }); the reveal intent matters because on mobile, or a saved YAML only layout, a plain editor open lands in the YAML pane where the nudge never renders. The card dot goes passive in select mode, same rule as the encryption button, and the table dot falls back to the passive indicator there too.

The editor's migration notice is also retinted to the migration purple so the identity is consistent from dashboard to nudge; the shared notice banner gains a --notice-accent hook that defaults to the old warning color, so the mac suffix and security notices are unchanged.

Verified in the browser on desktop and mobile viewports: all three surfaces land on the visual pane with the notice visible, Update config applies, and the notice clears.

Followup to #1652.

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.
  • pnpm run lint passes.
  • pnpm run test passes.
  • Tests have been added to verify that the new code works (where applicable).

@bdraco

bdraco commented Aug 15, 2026

Copy link
Copy Markdown
Member Author

@esphbot review

@github-actions github-actions Bot added the enhancement Improvement to an existing feature label Aug 15, 2026
@bdraco
bdraco marked this pull request as ready for review August 15, 2026 16:32
Copilot AI lite review requested due to automatic review settings August 15, 2026 16:32

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 makes the “config migration available” indicators (device card dot, table dot, and drawer badge) actionable so they open the device editor in a “reveal” mode that surfaces the migration notice reliably (not landing in a YAML-only pane on mobile). It also rethemes the editor’s migration notice to match the same purple accent used on the dashboard.

Changes:

  • Convert migration indicators into buttons that emit a dedicated open-config-migration event and prevent parent click/row activation.
  • Add reveal support to editDevice() and bind open-config-migration to editDevice(device, { reveal: true }) across card grid, table, and drawer.
  • Introduce --notice-accent in shared notice banner styles and set it for the config migration notice; add a new English tooltip/ARIA string for the action label.

Reviewed changes

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

Show a summary per file
File Description
test/components/device-card-migration-dot.test.ts Updates tests to assert the card migration dot is actionable, emits open-config-migration, and is passive in select mode.
test/components/dashboard/table-columns.test.ts Adds assertions that the table migration dot deep-links to the editor and stops row click; checks passive rendering in select mode.
test/components/dashboard/device-drawer-content-migration.test.ts Updates tests to expect the drawer migration badge to be a button that emits open-config-migration with device detail.
src/translations/en.json Adds an action-oriented label key for the migration indicator tooltip/ARIA label.
src/components/device/notice-banner.styles.ts Adds --notice-accent hook so notices can be retinted without changing default warning styling.
src/components/device/config-migration-notice.ts Sets --notice-accent to the migration purple for the migration notice banner.
src/components/device-card/styles.ts Adds button reset/focus styling for the migration dot now that it’s clickable.
src/components/device-card/render-bits.ts Introduces renderMigrationDot() to render an actionable button (or passive indicator in select mode) and fire open-config-migration.
src/components/device-card.ts Switches card rendering to use renderMigrationDot(this) instead of inline span markup.
src/components/dashboard/table-columns.ts Renders an actionable migration dot button in the name cell (non-select mode) that dispatches open-config-migration with device detail.
src/components/dashboard/table-cell-styles.ts Adds button reset/focus styling for the clickable migration dot in table cells.
src/components/dashboard/render-content.ts Wires open-config-migration to open the editor with { reveal: true } from cards, table, and drawer.
src/components/dashboard/device-drawer-content.ts Converts the migration badge to a button that fires open-config-migration.
src/components/dashboard/actions.ts Extends editDevice() to accept { reveal?: boolean } and append ?reveal=1 when requested.

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

Comment on lines +190 to +195
? html`<button
type="button"
class="status-badge status-badge--migration"
title=${this._localize("dashboard.status_migration_available_action")}
@click=${() => fireEvent(this, "open-config-migration", d)}
>
@bdraco

bdraco commented Aug 15, 2026

Copy link
Copy Markdown
Member Author

@esphbot review

Copilot AI review requested due to automatic review settings August 15, 2026 16:40

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 14 out of 14 changed files in this pull request and generated no new comments.

@esphbot

esphbot commented Aug 15, 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.

@bdraco

bdraco commented Aug 15, 2026

Copy link
Copy Markdown
Member Author

@esphbot review

Copilot AI review requested due to automatic review settings August 15, 2026 17:04

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 14 out of 14 changed files in this pull request and generated no new comments.

@esphbot

esphbot commented Aug 15, 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.

@bdraco

bdraco commented Aug 15, 2026

Copy link
Copy Markdown
Member Author

@esphbot review

Copilot AI review requested due to automatic review settings August 15, 2026 17:31

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 15 out of 15 changed files in this pull request and generated no new comments.

@esphbot

esphbot commented Aug 15, 2026

Copy link
Copy Markdown
Contributor

@bdraco: @esphbot review

Re-reviewed at current head. Both prior suggestions now resolved.

render-content-migration-wiring.test.ts pins /device/kitchen.yaml?reveal=1 across card, table, drawer — mirrors encryption precedent exactly. ::before inset -8px lifts 8px dots to 24px targets; td padding 11px/14px keeps extender unclipped.

One nit left: duplicate title plus aria-label, drawer badge. Non-blocking. Approving.

@esphbot

esphbot commented Aug 15, 2026

Copy link
Copy Markdown
Contributor

PR Review — Open the editor from the migration dot and badge

Merge-ready. Both open items from my previous pass are resolved in this revision; the only thing left is one a11y nit that is a judgement call, not a defect.

What's solid, re-verified against the tree:

  • The reveal plumbing targets the actual bug. editDevice now owns URL building via URLSearchParams; ?reveal=1device.ts:271 _pendingReveal (read independently of section, so the new section-less shape genuinely works) → _loadPreferenceslayoutRevealingAnchor("central", …), and buildDeviceUrl (device-url-state.ts:72) strips the param so a reload keeps the saved layout. Param order (section then reveal) is preserved, so the existing ?section=api&reveal=1 assertions still hold.
  • Event/propagation handling is correct on all three surfaces. The card button's e.stopPropagation() keeps _onClick (device-card.ts:394) from firing; _onKeydown bails unless composedPath()[0] === this, so Enter can't double-fire; dispatchRowEvent (table-columns.ts:76) stops propagation before fireEvent, and _onRowKeydown already bails on closest("button, a"). fireEvent is bubbling + composed, so each event reaches its render-content.ts binding.
  • Chrome resets are complete. The card and table dots get their own button.* resets; the drawer badge picks up the pre-existing button.status-badge reset (device-drawer-content/styles.ts:298), which already covers border/cursor/font-family with padding from .status-badge — nothing renders with native chrome.
  • Tap targets now clear 24px (8px dot + inset: -8px ::before), and td { padding: 11px 14px } means the extender is not clipped by overflow: hidden — this closes my prior sizing gap.
  • The wiring test I asked for is here. render-content-migration-wiring.test.ts mirrors the encryption precedent exactly (same mocks, same host stubs, makeConfiguredDevice()kitchen.yaml) and pins /device/kitchen.yaml?reveal=1 on card grid, table, and drawer plus the drawer close — the reveal intent can no longer regress silently.
  • The --notice-accent hook is correctly scoped. All three noticeBannerStyles consumers own separate shadow roots and none nest, .cta was already --esphome-primary (so it doesn't clash with the purple), and the var(--notice-accent, var(--esphome-warning, #f59e0b)) fallback leaves the mac-suffix and security notices byte-identical. --esphome-migration (#a855f7) is defined in both public/index.html:141 and public/web/index.html:44, and actually improves icon contrast on light backgrounds vs the amber it replaces (~4.0:1 vs ~2.4:1).
  • Select-mode passivity matches the card's encryption rule, and device-table.ts:255 rebuilds columns on selectMode change so the branch really re-renders. status_migration_available is still used by all three passive branches, so the new key orphans nothing. No file crosses the 500-line cap (table-columns.ts lands at ~497).

Key point:

  • Drawer badge duplicates the action string in both title and aria-label — double-announce nit, both shapes already exist in this repo; pick one deliberately.

Not verified: the branch isn't fetchable from this checkout, so the new tests were not executed — findings come from reading the diff against main plus the surrounding source.


🟢 Suggestions

1. Drawer badge repeats the same string in `title` and `aria-label`
src/components/dashboard/device-drawer-content.ts:193-196

The drawer badge now carries the identical action string twice: once as title, once as aria-label.

Why it matters (mildly): aria-label becomes the accessible name and title becomes the accessible description, so some AT stacks announce "Config migration available. Open the editor to update." and then repeat the same sentence as the description. Verified the name still starts with the visible text (Config migration available), so WCAG 2.5.3 Label in Name holds — this is a double-announce nit, not a defect.

Both shapes exist in the tree: table-columns.ts:276-282 (encryption button) pairs title + aria-label the same way, while the drawer's own encryption badge (device-drawer-content/render-sections.ts:66-76) is title-only. Either is defensible; dropping title here would match the drawer's neighbouring badge, keeping it matches the table. Flagging so the choice is deliberate.

This also supersedes Copilot's comment on line 198 — the aria-label it asked for is already present in this diff.

title=${this._localize("dashboard.status_migration_available_action")}
aria-label=${this._localize(
  "dashboard.status_migration_available_action"
)}

Checklist

  • Reveal intent is consumed and stripped from the URL
  • Click handling doesn't leak to card/row activation
  • Keyboard activation doesn't double-fire the row/card
  • All three surfaces are wired to a dashboard handler
  • New interactive controls have accessible names — suggestion #1
  • Pointer/tap target sizing for new controls (24px via ::before)
  • Button chrome reset covers all new buttons, incl. drawer badge
  • CSS custom-property hook is scoped, other notice consumers unchanged
  • No orphaned translation keys; new copy in en.json only
  • Test coverage pins the reveal deep-link on all three surfaces
  • Diff matches PR description, no scope creep

Automated review by Kōan (Claude) HEAD=af0f6de 4 min 5s

@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 merged commit dbefe52 into main Aug 15, 2026
13 checks passed
@bdraco
bdraco deleted the migration-dot-click branch August 15, 2026 18:49
@github-actions github-actions Bot locked and limited conversation to collaborators Aug 17, 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