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

fix(p-as-heading): p-as-heading rule to account for textContent length #3145

Merged
merged 8 commits into from
Oct 4, 2021

Conversation

Zidious
Copy link
Contributor

@Zidious Zidious commented Sep 3, 2021

Ref: #3130

@straker
Copy link
Contributor

straker commented Sep 16, 2021

@WilcoFiers we had questions on this requirement:

I would say that if the heading is longer than the paragraph, we pass it. If it's less than twice as long, we set it for review, and only if the suspected fake heading is at least 200% shorter than the paragraph do we allow it to be failed.

There's a gap in the algorithm, like what happens if it's more than twice as long but not greater than the length. Could you give a bit more clarification? Here's what I have reading that description:

const headingLength = headingEl.textContent.length;
const paragraphLength = pEl.textContext.length;

if (headingLength > paragraphLength) {
  return true;
}
else if (headingLength < paragraphLength / 2) {
  return false;
} 
else {
  return undefined;
}

@WilcoFiers
Copy link
Contributor

@straker I think your dummy code is pretty much how I think of it. We should probably put in options to make these values configurable. Beyond that, these seem like reasonable heuristics for this rule.

@Zidious
Copy link
Contributor Author

Zidious commented Sep 23, 2021

@straker I think your dummy code is pretty much how I think of it. We should probably put in options to make these values configurable. Beyond that, these seem like reasonable heuristics for this rule.

@WilcoFiers Could you provide a bit more info on configurable by options?

@Zidious Zidious marked this pull request as ready for review September 23, 2021 14:02
@Zidious Zidious requested a review from a team as a code owner September 23, 2021 14:02
lib/checks/navigation/p-as-heading-evaluate.js Outdated Show resolved Hide resolved
test/checks/navigation/p-as-heading.js Show resolved Hide resolved
test/checks/navigation/p-as-heading.js Outdated Show resolved Hide resolved
Copy link
Contributor

@WilcoFiers WilcoFiers left a comment

Choose a reason for hiding this comment

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

One comment, rest looks good.

lib/checks/navigation/p-as-heading.json Show resolved Hide resolved
@Zidious Zidious changed the title fix: p-as-heading rule to account for textContent length fix(p-as-heading): p-as-heading rule to account for textContent length Oct 1, 2021
doc/check-options.md Outdated Show resolved Hide resolved
doc/check-options.md Outdated Show resolved Hide resolved
Zidious and others added 2 commits October 1, 2021 17:14
Co-authored-by: Wilco Fiers <[email protected]>
Co-authored-by: Wilco Fiers <[email protected]>
@Zidious Zidious merged commit 400a230 into develop Oct 4, 2021
@Zidious Zidious deleted the headingrulechange branch October 4, 2021 15:56
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.

3 participants