Skip to content

core: add FormElements gatherer#11062

Merged
devtools-bot merged 143 commits into
GoogleChrome:masterfrom
makunde:formGatherer
Aug 5, 2020
Merged

core: add FormElements gatherer#11062
devtools-bot merged 143 commits into
GoogleChrome:masterfrom
makunde:formGatherer

Conversation

@makunde

@makunde makunde commented Jul 6, 2020

Copy link
Copy Markdown
Contributor

Summary

This PR will create a new gatherer that collects attributes that are important for creating an audit that will evaluate how well a developer uses autocomplete. More information is seen in the documentation linked and the related issue

feature

This is for a new form gatherer that collects form elements so we can perform audits that will evaluate how will it used autocomplete.

https://docs.google.com/document/d/1yiulNnV8uEy1jPaAEmWeHxHcQOzxpqvAV4hOFpXLJ1M/edit#

Related Issues/PRs

Issue #10450

@makunde makunde requested a review from a team as a code owner July 6, 2020 21:39
@makunde makunde requested review from Beytoven and removed request for a team July 6, 2020 21:39

@patrickhulce patrickhulce left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

mostly the ts-ignore cleanup, but the impl still looks great!

Comment thread lighthouse-cli/test/fixtures/form.html Outdated
Comment thread lighthouse-core/gather/gatherers/form-elements.js Outdated
Comment thread lighthouse-core/gather/gatherers/form-elements.js Outdated
Comment thread lighthouse-core/gather/gatherers/form-elements.js Outdated
Comment thread lighthouse-core/gather/gatherers/form-elements.js Outdated
Comment thread lighthouse-core/gather/gatherers/form-elements.js Outdated
});
}
if (child instanceof HTMLLabelElement) {
// @ts-ignore

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

this one shouldn't be needed either, what was going on?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

without it, it gives me a type error "Object is possibly 'undefined'."

Comment thread lighthouse-core/gather/gatherers/form-elements.js Outdated
Comment thread lighthouse-core/gather/gatherers/form-elements.js Outdated
Co-authored-by: Patrick Hulce <patrick.hulce@gmail.com>
@vercel vercel Bot temporarily deployed to Preview August 3, 2020 20:32 Inactive
makunde and others added 12 commits August 3, 2020 16:32
Co-authored-by: Patrick Hulce <patrick.hulce@gmail.com>
Co-authored-by: Patrick Hulce <patrick.hulce@gmail.com>
Co-authored-by: Patrick Hulce <patrick.hulce@gmail.com>
Co-authored-by: Patrick Hulce <patrick.hulce@gmail.com>
Co-authored-by: Patrick Hulce <patrick.hulce@gmail.com>
Co-authored-by: Patrick Hulce <patrick.hulce@gmail.com>
Co-authored-by: Patrick Hulce <patrick.hulce@gmail.com>

@patrickhulce patrickhulce left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

LGTM 🎉

Woohoo congratulations on crossing the finish line here @gMakunde! :)

Comment thread lighthouse-core/gather/gatherers/form-elements.js Outdated
Comment thread lighthouse-core/gather/gatherers/form-elements.js Outdated
Comment thread lighthouse-core/gather/gatherers/form-elements.js Outdated
makunde and others added 4 commits August 4, 2020 15:21
Co-authored-by: Patrick Hulce <patrick.hulce@gmail.com>
Co-authored-by: Patrick Hulce <patrick.hulce@gmail.com>
Co-authored-by: Patrick Hulce <patrick.hulce@gmail.com>
@googlebot

Copy link
Copy Markdown

A Googler has manually verified that the CLAs look good.

(Googler, please make sure the reason for overriding the CLA status is clearly documented in these comments.)

ℹ️ Googlers: Go here for more info.

@connorjclark connorjclark left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

one last thing then LGTM!

@googlebot

Copy link
Copy Markdown

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

@connorjclark

Copy link
Copy Markdown
Collaborator

cla bot is asking many many people from other PRs merged to master for consent. Ignoring it.

@googlebot

Copy link
Copy Markdown

A Googler has manually verified that the CLAs look good.

(Googler, please make sure the reason for overriding the CLA status is clearly documented in these comments.)

ℹ️ Googlers: Go here for more info.

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.