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

core(tap targets): make tap targets failures more visible #7373

Merged
merged 4 commits into from
Mar 5, 2019

Conversation

mattzeunert
Copy link
Contributor

Summary

Right now if the audit score is above 90 the tap targets audit doesn't fail even if there are overlap failures. That way it's hard to identify what's dragging the SEO score down. So if there are failures we should cap the score at 89.

Related Issues/PRs

Closes #7338

@mattzeunert mattzeunert changed the title Make tap targets failures more visible core(tap targets): make tap targets failures more visible Mar 4, 2019
Copy link
Collaborator

@patrickhulce patrickhulce left a comment

Choose a reason for hiding this comment

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

WFM

passingTapTargetRatio = (passingTapTargetCount / tapTargetCount);
// If there are any failures then we don't want the audit to pass,
// so keep the score below 90.
score = Math.min(passingTapTargetRatio * 0.9, 0.89);
Copy link
Collaborator

Choose a reason for hiding this comment

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

do we need to do both? isn't just doing 1 of these sufficient?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah just * 0.89 is enough.

I was trying to interpret this and thought maybe you meant that multiplying by 0.9 gives a smoother score or something.

I'd say go with * .9/Math.min(score, .89)/targets.length ? 0 : 1 if each and every failure should be looked at.

Copy link
Collaborator

Choose a reason for hiding this comment

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

oh yeah was just trying to suggest using one of those not in combination :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ahh, it makes sense now 🙂

Copy link
Member

@brendankenny brendankenny left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@exterkamp exterkamp left a comment

Choose a reason for hiding this comment

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

Forgot to leave a review, but I did look at this lol

LGTM!

@paulirish paulirish merged commit d3c155e into master Mar 5, 2019
@paulirish paulirish deleted the more-visible-tap-targets-failures branch March 5, 2019 19:45
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.

5 participants