Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enable categories other than a11y to have audit groups #4093

Closed
rviscomi opened this issue Dec 19, 2017 · 15 comments
Closed

Enable categories other than a11y to have audit groups #4093

rviscomi opened this issue Dec 19, 2017 · 15 comments
Assignees

Comments

@rviscomi
Copy link
Member

Right now only the Accessibility audit category can have groups of audits. Make this feature reusable so other categories like SEO can have groups.

Context: #4057 (comment)

@paulirish
Copy link
Member

paulirish commented Jan 9, 2018

this'll probably mean a decent refactor within category-renderer.

one approach is that we kill _renderAccessibilityCategory and improve the extensibility of _renderDefaultCategory
done generally, it probably means that we enable the notApplicable group for all other categories too.

seems good!

@kdzwinel
Copy link
Collaborator

kdzwinel commented Jan 18, 2018

@paulirish I'm merging a11y category renderer with the default one. a11y renderer doesn't have the 'X Failed Audits' header. Should we:

  1. have it in all audit categories,
  2. remove it from all audit categories,
  3. have it only for non-grouped audit categories (as it is now)

?

IMO it's redundant and we could skip it everywhere for consistency and to save some space. E.g. PWA section with an w/o the header:

screen shot 2018-01-18 at 00 59 21

@paulirish
Copy link
Member

Yeah I like nuking the heading (and indentation).

@vinamratasingal does that work for you?

@vinamratasingal-zz
Copy link

I actually like keeping the failed audits section- it creates consistency between the other sections. Paul/Konrad- any reasons why you like nuking the header?

cc: @hwikyounglee in case she wants to add her perspective :)

@hwikyounglee
Copy link

Header gives a visual help to group/separate things. I suggest we keep it.

@kdzwinel
Copy link
Collaborator

IMO failed audits are clearly marked (by the description, ❌ and, in many cases, red debugString) and don't require any more introduction. Removing 'X Failed Audits' header saves us some horizontal space that, especially inside of DevTools, is very precious.

If we decide to keep them, we should probably still deal with the inconsistency between sections that use groups ('Accessibility', 'SEO') and sections that don't use them ('PWA', 'Best Practices'). Should we add headers to all of them?

@vinamratasingal-zz
Copy link

I understand that the failed audits have the X, but I still think for consistency/visual help perspective, we should keep it in there. I suggest we push this paradigm across all our sections.

cc @paulirish @hwikyounglee

@kdzwinel
Copy link
Collaborator

OK, I see your point👍 I made a quick change to show how the accessibility section will look like with this header:

header

@vinamratasingal-zz
Copy link

Can we add in a dropdown button for the Failed Audits section too?

@hwikyounglee @paulirish does the above LGTM to you?

@kdzwinel
Copy link
Collaborator

By the dropdown you mean the ability to collapse the failed audits group? Yes, we can add it. ATM it's explicitly disabled.

@vinamratasingal-zz
Copy link

Yeah... I'm also happy to put this on the next Lighthouse meeting docket if folks have strong opinions on this/would prefer to have an in-person discussion to talk through their concerns.

@patrickhulce
Copy link
Collaborator

Yeah discussing in next meeting sounds good, I was in favor of nuking as well :)

Biggest reason to nuke IMO is that the extra indentation is a waste of precious space in DevTools but honestly that issue is much bigger than just this

@hwikyounglee
Copy link

If not urgent, one way is to make the formatting cleanup as a new issue assigned to me. I just created one (#4284), and don't know how to assign this to myself 🤔. Could anyone help? Thanks!

@kdzwinel
Copy link
Collaborator

I'm not sure if the header discussion should be a blocker for the main task here (I have a PR ready). WDYT about keeping the status quo for the header ('X Failed Audits' shown only for categories w/o groups) and proceeding with making groups available for the SEO category? When #4284 will be ready, I'll be happy to go back and work on the report page adjustments as a separate task.

@paulirish
Copy link
Member

sg. let's go ahead with whats in your PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants