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

report: remove href from category score gauge #13180

Merged
merged 3 commits into from
Oct 6, 2021
Merged

report: remove href from category score gauge #13180

merged 3 commits into from
Oct 6, 2021

Conversation

connorjclark
Copy link
Collaborator

fixes #13170

@@ -333,7 +333,7 @@ export class CategoryRenderer {
renderScoreGauge(category, groupDefinitions) { // eslint-disable-line no-unused-vars
const tmpl = this.dom.createComponent('gauge');
const wrapper = this.dom.find('a.lh-gauge__wrapper', tmpl);
this.dom.safelySetHref(wrapper, `#${category.id}`);
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

alternative: remove href from

<a href="#" class="lh-gauge__wrapper">

Copy link
Member

@paulirish paulirish left a comment

Choose a reason for hiding this comment

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

this works.

I'll put up a PR building on this one to replace #13016

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.

I thought we only wanted to remove the href for itself in its own category, not remove it for all gauges?

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.

@paulirish seems like you have a plan for this, shall I designate you as the reviewer here?

@@ -549,7 +549,7 @@

<!-- Lighthouse score gauge -->
<template id="gauge">
<a href="#" class="lh-gauge__wrapper">
<a class="lh-gauge__wrapper">
Copy link
Collaborator

Choose a reason for hiding this comment

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

should these even be anchors then?

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

Successfully merging this pull request may close these issues.

Don't set href in main category gauges
4 participants