fix(best-practices): respect existing heading in #bp-summary#5211
Draft
marcoscaceres wants to merge 13 commits intomainfrom
Draft
fix(best-practices): respect existing heading in #bp-summary#5211marcoscaceres wants to merge 13 commits intomainfrom
marcoscaceres wants to merge 13 commits intomainfrom
Conversation
The div has aria-label with the full description, so children are presentational. Set inner img alt to empty to avoid double-announcement.
Agent-Logs-Url: https://github.com/speced/respec/sessions/391940a3-1f50-400e-849b-39f06e39e7bb Co-authored-by: marcoscaceres <870154+marcoscaceres@users.noreply.github.com>
Agent-Logs-Url: https://github.com/speced/respec/sessions/6ae7a1ff-7486-4388-b079-084233604857 Co-authored-by: marcoscaceres <870154+marcoscaceres@users.noreply.github.com>
The nullish coalescing operator treats "" as non-nullish, so an empty-string version would display as blank instead of the em dash fallback. The ternary correctly maps falsy values (including "") to the fallback.
Avoid injecting a duplicate heading when #bp-summary already has one, and use prepend() so the generated heading always appears at the top. Closes #5176
Contributor
There was a problem hiding this comment.
Pull request overview
Fixes heading injection behavior in the Best Practices summary section so it doesn’t create duplicate headings when authors provide their own, and adjusts Can I Use rendering/tests for improved HTML validity/accessibility labeling.
Changes:
core/best-practices: avoid injecting an extra “Best Practices Summary” heading when#bp-summaryalready has a heading; insert generated heading at the top viaprepend().core/caniuse: addrole="img"to support cells, move the accessible name to the parent viaaria-label, and make inner logoaltempty; adjust aria-label/title wording for unknown versions.- Add/adjust spec coverage for both behaviors.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| tests/spec/core/caniuse-spec.js | Updates expectations for role="img", empty alt, and “(version unknown)” aria-label wording. |
| tests/spec/core/best-practices-spec.js | Adds a regression test ensuring an existing #bp-summary heading isn’t duplicated. |
| src/core/caniuse.js | Implements role="img" + aria-label-based accessible naming and updates version wording. |
| src/core/best-practices.js | Skips injecting the generated heading when an existing heading is present; prepends generated heading. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
querySelector for existing headings in #bp-summary was missing h1, so a pre-existing <h1> inside #bp-summary would not suppress the injected heading. Extend selector to h1–h6.
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Contributor
|
Just as a heads up, I was blocked by some firewall rules while working on your feedback. Expand below for details. Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
Contributor
Author
|
@copilot can you take a look at CI and fix the issue? |
Contributor
Fixed in b823c8c. I checked the failing CI run ( Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
When
#bp-summaryalready contained a heading,best-practices.jswould inject an additional<h1>Best Practices Summary</h1>at the end of the section — producing a duplicate heading below the existing one.Fix
h1–h6before injecting the generated heading.prepend()instead ofappendChild()so the generated heading always appears at the top of the section.Other changes
core/caniuse.js: Minor TypeScript type annotation improvements (@typedefforCaniuseResult/BrowserCellReducer, narrowing casts to avoid implicitany). No behaviour changes.Testing
Added a new test: "does not duplicate heading when bp-summary already has one".
Closes #5176