report: add label to provide context for subItems#13858
report: add label to provide context for subItems#13858connorjclark wants to merge 12 commits intomainfrom
Conversation
IMO yes. Would underline work? |
| const str_ = i18n.createMessageInstanceIdFn(__filename, UIStrings); | ||
|
|
||
| class AxeAudit extends Audit { | ||
| static get relatedNodesLabel() { |
There was a problem hiding this comment.
static field or function instead of a getter?
There was a problem hiding this comment.
Well, this does match how we handle meta.
| subItems: axeNode.relatedNodes.length ? { | ||
| type: 'subitems', | ||
| items: axeNode.relatedNodes.map(node => ({relatedNode: Audit.makeNodeItem(node)})), | ||
| label: this.relatedNodesLabel, |
There was a problem hiding this comment.
haha, this kind of thing makes me think axe-audit inheritance should just be killed and turned into a function call :)
| /** Label of a table column that identifies HTML elements that have failed an audit. */ | ||
| failingElementsHeader: 'Failing Elements', | ||
| /** Label of a table column that identifies HTML elements that are related to a failure in an audit. */ | ||
| relatedElementsHeader: 'Related Elements', |
There was a problem hiding this comment.
singular/plural for this is somewhat unfortunate. "Reasons" (when there's only one reason) seems worse to me than e.g. "Duplicate Elements" for some reason.
Could maybe make it the count an argument and only do a single option for cases that are always singular? I don't know if TC or whatever will complain/get weird with {relatedNodeCount, plural, other {Background Element}}, though.
| ]); | ||
| expect(result.items).toHaveLength(1); | ||
| expect(result.items[0]).toMatchInlineSnapshot(` | ||
| Object { |
There was a problem hiding this comment.
indentation issue with these files?
There was a problem hiding this comment.
oops. You have to babysit jest when asking it to -u inline snapshots. It was fixed for a short minute... but regressed at some point. Maybe fixed in latest, idk.
yeah.. i tried out some styles to shrink the screenshot + constrain the text for subrows. it's.. better. |
paulirish
left a comment
There was a problem hiding this comment.
these capitals feel extraneous. (though perhaps we adopted a style for this already?)
i don't have a great idea on the label's typography or graphical delineation...
@Jiwoong can you help us out?
- we're adding this label to communicate what these sub items are all about. it looks aiight but you could probably help us with it
- especially when theres more thumbnails in the sub items, the layout gets pretty wonky and hard to follow.. especially tricky cuz we autosize based on aspect ratio and the aspect ratios are totally unpredictable. in an above comment i tried shrinking subitem thumbnails. any ideas?
| /** Label of a table column that identifies HTML elements that have failed an audit. */ | ||
| failingElementsHeader: 'Failing Elements', | ||
| /** Label of a table column that identifies HTML elements that are related to a failure in an audit. */ | ||
| relatedElementsHeader: 'Related Elements', |
There was a problem hiding this comment.
| relatedElementsHeader: 'Related Elements', | |
| relatedElementsHeader: 'Related elements', |
There was a problem hiding this comment.
yeah but Failed Elements :( change that too? it's the (regular) column text
| description: 'Low-contrast text is difficult or impossible for many users to read. ' + | ||
| '[Learn more](https://web.dev/color-contrast/).', | ||
| /** Label of a table column that identifies a single HTML element that is the background of another HTML element. */ | ||
| backgroundElementHeader: 'Background Element', |
There was a problem hiding this comment.
| backgroundElementHeader: 'Background Element', | |
| backgroundElementHeader: 'Background element', |
| /** Description of a Lighthouse audit that tells the user *why* they should try to pass. This is displayed after a user expands the section to see more. No character length limits. 'Learn More' becomes link text to additional documentation. */ | ||
| description: 'The value of an ARIA ID must be unique to prevent other instances from being overlooked by assistive technologies. [Learn more](https://web.dev/duplicate-id-aria/).', | ||
| /** Label of a table column that identifies HTML elements that are duplicates of another HTML element. */ | ||
| relatedElementsHeader: 'Duplicate Elements', |
There was a problem hiding this comment.
| relatedElementsHeader: 'Duplicate Elements', | |
| relatedElementsHeader: 'Duplicate elements', |
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
made the line thinner (pictures have been updated) |
|
we don't like how we show the sub-row label multiple times let's add the label to be just below the main header label, but like this: and lets also add that vertical line to the sub row items (like already done for insight audit tables) |





This adds a field
labeltoTableSubItems, to help improve context when rendering subitems underneath a table item row.Did not give
third-party-weba subitems label because it seems good alreadycnn report here: https://lighthouse-1065v6gec-googlechrome.vercel.app/gh-pages/viewer/?gist=8fdec0793bab3abea4e66bc1129b06d7