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

feat(headings): add warnings for hidden heading levels #140

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ericrallen
Copy link

I wasn't entirely sure what the resolution for #121 should be, or why there would be hidden heading levels on the page, but this update treats a heading with aria-hidden or that meets the criteria for the :hidden selector as a Warning and styles things differently for them.

Submitted as a Draft PR because I'm not even sure if this is something that should be added or if you'd prefer a more robust solution that adds a Warnings tab to the Info Panel.

Closes #121

@khanbot
Copy link

khanbot commented Mar 16, 2019

CLA signature looks good 👍

@somewhatabstract
Copy link
Contributor

I haven't forgotten this. I will try to make time this week to give feedback. Thank you for your patience.

@ericrallen
Copy link
Author

Sure thing. I just picked this one up because I had some free time and saw an issue I thought I could handle.

Like I said, it might not even be worth pursuing.

Copy link
Contributor

@somewhatabstract somewhatabstract left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a nice clean change.

It needs to be merged up with the latest version of master of course, but otherwise looks great.

</div>
`,
errorLevel: "warning"
};
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Having the idea of warnings for this makes sense. I mean, hidden headers could be a mistake or it could be intentional.

I wonder if we should add a feature to control what level of issue we annotate? Like a cumulative level thing that says "show all annotations of level X and above"? What do you think? (not suggesting you do that as part of this PR, just thinking aloud).

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that definitely makes sense if you introduce levels.

It could work sort of like the Console log-level filtering in your browser's Developer Tools.

It might even make sense to have each plugin be able to define what its defaults are and fall back to some global default of ["error", "warning"] if no levels are specified by the plugin.

@ericrallen ericrallen marked this pull request as ready for review May 19, 2019 13:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Headings - Indicates a heading even when set as display:none
3 participants