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(audits): Point a11y audit links to web.dev #9084

Merged
merged 2 commits into from
Jul 25, 2019
Merged

core(audits): Point a11y audit links to web.dev #9084

merged 2 commits into from
Jul 25, 2019

Conversation

mfriesenhahn
Copy link
Collaborator

Points "Learn more" links in accessibility audits to relevant web.dev guides.

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.

These are great! 🎉

I especially love the example glitch.me embeds 😍 (example https://web.dev/custom-controls-labels/)

@patrickhulce
Copy link
Collaborator

patrickhulce commented May 30, 2019

(You'll need to update our fixtures for tests though @mfriesenhahn, should just be running yarn update:sample-json)

@brendankenny
Copy link
Member

@paulirish @egsweeny how do y'all want to handle docs review?

@mfriesenhahn
Copy link
Collaborator Author

@brendankenny @patrickhulce I can't seem to make yarn update:sample-json happy. I'm getting the following error even though protobuf is installed:

$ protoc --python_out=./ ./proto/lighthouse-result.proto && mv ./proto/*_pb2.py ./proto/scripts || (echo "❌ Install protobuf ≥ 3.7.1 to compile the proto file." && false)
$ cd proto/scripts && PROTOCOL_BUFFERS_PYTHON_IMPLEMENTATION=cpp PROTOCOL_BUFFERS_PYTHON_IMPLEMENTATION_VERSION=2 python json_roundtrip_via_proto.py
Traceback (most recent call last):
  File "json_roundtrip_via_proto.py", line 5, in <module>
    import lighthouse_result_pb2 as lhr_pb2
  File "/Users/mfriesenhahn/Documents/GitHub/lighthouse/proto/scripts/lighthouse_result_pb2.py", line 7, in <module>
    from google.protobuf.internal import enum_type_wrapper
ImportError: No module named google.protobuf.internal
error Command failed with exit code 1.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.
error Command failed with exit code 1.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.

mfriesenhahn-macbookpro:lighthouse mfriesenhahn$ protoc --version
libprotoc 3.7.1

@brendankenny
Copy link
Member

I don't know what's causing that, but random searching did come up with the fact that there may be conflicting versions of protobuf installed.

What does pip list say for you?

<<insert other python environment debugging techniques that I have no clue about>>

cc @exterkamp

@mfriesenhahn
Copy link
Collaborator Author

mfriesenhahn commented Jun 7, 2019

OK, I realized I didn't have the python environment set up correctly, so after some fiddling, here's what I'm getting for pip list:

Package    Version
---------- -------
pip        19.0.3 
protobuf   3.7.1  
setuptools 40.8.0 
six        1.12.0 
wheel      0.33.1 

And here's what I get when I run yarn update:sample-json:

$ protoc --python_out=./ ./proto/lighthouse-result.proto && mv ./proto/*_pb2.py ./proto/scripts || (echo "❌ Install protobuf ≥ 3.7.1 to compile the proto file." && false)
$ cd proto/scripts && PROTOCOL_BUFFERS_PYTHON_IMPLEMENTATION=cpp PROTOCOL_BUFFERS_PYTHON_IMPLEMENTATION_VERSION=2 python json_roundtrip_via_proto.py
✨  Done in 6.58s.

Seems like travis is happy now?

@brendankenny
Copy link
Member

Seems like travis is happy now?

indeed!

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.

L8 to this party 🎉, LGTM love the new links, docs look fantastic! Sorry I missed the protobuf troubles! Glad it worked out!

@patrickhulce
Copy link
Collaborator

@paulirish @egsweeny how do y'all want to handle docs review?

Bump on this, how do y'all want to handle the review here? My expectation would be that the review happens in the web.dev repo, and does not block this PR, but want to double check.

@patrickhulce
Copy link
Collaborator

Apologies @mfriesenhahn but this will need a rebase.

@brendankenny
Copy link
Member

the rebase was kind of tricky because of the i18n changes, so I pushed it for you. A diff shows nothing changed (except to the en-US.json format, which is expected).

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.

Bump on this, how do y'all want to handle the review here? My expectation would be that the review happens in the web.dev repo, and does not block this PR, but want to double check.

Yeah, now that #9114 is landed, changes to urls like this aren't going to cause translation issues, so the question is just the web.dev content.

We talked about doing the review of the web.dev content from Lighthouse's perspective in these PRs, but that hasn't worked out so far :/ Maybe we can move forward on these since it's content well known beyond LH and we could save the blocking reviews for the metrics, etc?

Meanwhile these articles LGTM! :)

@patrickhulce
Copy link
Collaborator

SGTM! landing time? 🕐

@paulirish
Copy link
Member

yup landing this wfm.

tbh i'll take a closer look at the other categories.

@paulirish paulirish merged commit 7e26001 into GoogleChrome:master Jul 25, 2019
@mfriesenhahn
Copy link
Collaborator Author

Thanks, all. I'll continue submitting PRs for the remaining categories as I can squeeze them in :)

@mfriesenhahn mfriesenhahn deleted the a11y-audit-links branch July 26, 2019 19:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants