report: add Trust & Safety group under Best Practices#10623
Conversation
patrickhulce
left a comment
There was a problem hiding this comment.
looks great @Beytoven thanks! :)
| /** Title of the Trust & Safety group of the Best Practices category. Within this section are the audits related to trust and safety. */ | ||
| safetyGroupTitle: 'Trust & Safety', | ||
| /** Title of the General group of the Best Practices category. Within this section are the audits that don't belong to a specific group. */ | ||
| generalGroupTitle: 'General', |
There was a problem hiding this comment.
| generalGroupTitle: 'General', | |
| bestPracticesGeneralGroupTitle: 'General', |
I like that the others prefix with the category, especially nice for a group titled General ;)
| /** Title of the Best Practices category of audits. This is displayed at the top of a list of audits focused on topics related to following web development best practices and accepted guidelines. Also used as a label of a score gauge; try to limit to 20 characters. */ | ||
| bestPracticesCategoryTitle: 'Best Practices', | ||
| /** Title of the Trust & Safety group of the Best Practices category. Within this section are the audits related to trust and safety. */ | ||
| safetyGroupTitle: 'Trust & Safety', |
There was a problem hiding this comment.
| safetyGroupTitle: 'Trust & Safety', | |
| bestPracticesSafetyGroupTitle: 'Trust & Safety', |
| {id: 'image-aspect-ratio', weight: 1}, | ||
| {id: 'image-size-responsive', weight: 1}, | ||
| // Trust & Safety | ||
| {id: 'is-on-https', weight: 1, group: 'trust-and-safety'}, |
There was a problem hiding this comment.
per #10619 I think we want to add redirects-http too?
There was a problem hiding this comment.
I didn't realize we were moving one of the PWA audits as well. @connorjclark can you confirm?
There was a problem hiding this comment.
Let's have redirects-http be in PWA and this new subgroup.
There was a problem hiding this comment.
Oh yes definitely don't move, I just meant also have it show up here :)
|
are ungrouped audits still a thing? Is it better to have everything else under 'General' or to just have them be ungrouped? I don't have a strong opinion, but generally using up UI space just to have a generic "other things" title doesn't add that much. OTOH maybe it doesn't look so great not grouped :) |
| {id: 'image-size-responsive', weight: 1}, | ||
| // Trust & Safety | ||
| {id: 'is-on-https', weight: 1, group: 'trust-and-safety'}, | ||
| {id: 'external-anchors-use-rel-noopener', weight: 1, group: 'trust-and-safety'}, |
There was a problem hiding this comment.
how would you feel about best-practices-trust-and-safety? right now basically every group prefixes the category or an abbreviation of it
oh, looks like ungrouped ones come first which wouldn't be great for this. What great ascii art someone put in that comment, though! I guess the order could be reversed, but OTOH here is @patrickhulce arguing for the exact opposite of "minimum amount of groups pls" :) |
The styling doesn't look too good with a mix of grouped and ungrouped audits. For reference: |
…r gatherers w/ properties that have a .bind() function
brendankenny
left a comment
There was a problem hiding this comment.
agreed with @patrickhulce this looks just about ready to go!
| title: str_(UIStrings.seoCrawlingGroupTitle), | ||
| description: str_(UIStrings.seoCrawlingGroupDescription), | ||
| }, | ||
| 'best-practices-safety': { |
There was a problem hiding this comment.
maybe best-practices-trust-safety?
If I come in looking for the Trust and Safety group, my ctrl-f would definitely start with "trust" :)
|
|
||
| assert.equal(config.passes.length, 1, 'did not filter config'); | ||
| assert.deepStrictEqual(config, extendedConfig, 'had mutations'); | ||
| expect(config).toMatchObject(extendedConfig, 'had mutations'); |
There was a problem hiding this comment.
toMatchObject here only checks that everything in extendedConfig is in config, which means config can be extendedConfig plus extra non-matching properties and this assertion would still pass.
e.g. if one of the gatherer.implementation in extendedConfig accidentally got switched to {}, it would match with almost anything in the same gatherer.implementation slot in config.
This could be checked both ways (each is a subset of the other) with
expect(config).toMatchObject(extendedConfig, 'had mutations');
expect(extendedConfig).toMatchObject(config, 'had mutations');to check both directions but I think we really want
expect(config).toEqual(extendedConfig, 'had mutations');to make sure we're being strict on equality.
There was a problem hiding this comment.
oh good catch, while we're on the subject I don't think the message part works with that particular expect API, so we can nix had mutations or convert it to a comment and stick to the actual object diff
Co-Authored-By: Brendan Kenny <bckenny@gmail.com> Co-Authored-By: Patrick Hulce <patrick.hulce@gmail.com>
patrickhulce
left a comment
There was a problem hiding this comment.
LGTM, nicely done @Beytoven! 🎉
Co-Authored-By: Patrick Hulce <patrick.hulce@gmail.com>
|
@brendankenny did you wish to have another look or are you fine with my LGTM? |
brendankenny
left a comment
There was a problem hiding this comment.
yes, sorry to barge in, LGTM2!
|
cool! FYI @Beytoven you might need to merge master to fix the required statuses |
As the title suggests, this PR simply moves the Trust and Safety related audits to a specified sub-category. All other Best Practices audits are moved under General.