Skip to content

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

Merged
Zidious merged 8 commits intodevelopfrom
headingrulechange
Oct 4, 2021
Merged

fix(p-as-heading): p-as-heading rule to account for textContent length#3145
Zidious merged 8 commits intodevelopfrom
headingrulechange

Conversation

@Zidious
Copy link
Copy Markdown
Contributor

@Zidious Zidious commented Sep 3, 2021

Ref: #3130

@straker
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
@Zidious Zidious requested a review from WilcoFiers September 23, 2021 14:02
Copy link
Copy Markdown
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.

@Zidious Zidious requested a review from WilcoFiers October 1, 2021 15:48
@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
Zidious and others added 2 commits October 1, 2021 17:14
Co-authored-by: Wilco Fiers <WilcoFiers@users.noreply.github.com>
Co-authored-by: Wilco Fiers <WilcoFiers@users.noreply.github.com>
@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