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

Accessibility and SEO improvements #540

Merged
merged 1 commit into from
May 5, 2021
Merged

Accessibility and SEO improvements #540

merged 1 commit into from
May 5, 2021

Conversation

jcabak
Copy link
Contributor

@jcabak jcabak commented May 2, 2021

  • Background and foreground colours have a sufficient contrast ratio. (In “Docs” for date label “Last modified”)
  • Links have a discernible name
  • Added aria-label for links “read more” and “next” and previous” article in blog
  • Links to cross-origin destinations are safe (added rel="noopener")
  • Document have a meta description
  • Text remains visible during webfont load (added font-display: swap;)

@LisaFC
Copy link
Collaborator

LisaFC commented May 5, 2021

This is great, thanks! I'll give it a proper look later.

@LisaFC LisaFC merged commit 604e27f into google:master May 5, 2021
{{ if and hugo.IsProduction (ne $outputFormat "print") -}}
{{ if and (eq (getenv "HUGO_ENV") "production") (ne $outputFormat "print") -}}
Copy link
Collaborator

@chalin chalin Aug 9, 2021

Choose a reason for hiding this comment

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

@jcabak - why were the changes introduced by #413 reverted here?
@LisaFC - maybe this is the case of #611?

IMHO we should be using hugo.IsProduction, not checking environment variable values -- in particular since a production build can be requested using the -e flag command line flag.

/cc @nate-double-u

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm assuming that this (retrograde) change was accidental, but I could be wrong -- and thanks for clarifying the situation @jcabak when you have a minute. Let's carry the discussion over to #652.

Copy link
Contributor Author

@jcabak jcabak Aug 9, 2021

Choose a reason for hiding this comment

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

Honestly I am surprised that I changed this line, you have right @chalin it was accidental. I appreciate your vigilance 🤠

Copy link
Collaborator

@chalin chalin Aug 9, 2021

Choose a reason for hiding this comment

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

Thanks for promptly confirming @jcabak. FYI, #653 reintroduces use of hugo.IsProduction.

Comment on lines -22 to +23
{{ if hugo.IsProduction }}
{{ if eq (getenv "HUGO_ENV") "production" }}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Comment on lines +33 to +34
src="https://unpkg.com/lunr@2.3.8/lunr.min.js"
integrity="sha384-vRQ9bDyE0Wnu+lMfm57BlYLO0/XauFuKpVsZPs7KEDwYKktWi5+Kz3MP8++DFlRY"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Hi again @jcabak. Why was lunr's version retrograded?

Could it be that, aside from the addition of <meta name="description" ...>, every other change in this file is because you were working from an old version of the file (including the removal of the shortcodes.css link below)?

For some context, see https://github.com/google/docsy/pull/540/files#r685339766.

/cc @LisaFC @nate-double-u

@@ -1,7 +1,7 @@
<ul class="list-unstyled d-flex justify-content-between align-items-center mb-0 pt-5 d-print-none">
Copy link
Collaborator

Choose a reason for hiding this comment

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

@jcabak - what the removal of d-print-none voluntary? By doing so you're reverting #484.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry @chalin, I don't know why I made mistakes, especially with removal stylesheet for tabbed pane and d-print-none. It shouldn't be like that.

crossorigin="anonymous"></script>
{{end}}
{{ if .Site.Params.prism_syntax_highlighting }}
<!-- stylesheet for Prism -->
<link rel="stylesheet" href="{{ "/css/prism.css" | relURL }}"/>
{{ end }}
<!-- stylesheet for tabbed pane -->
<link rel="stylesheet" href="{{ "/css/shortcodes.css" | relURL }}"/>
Copy link
Collaborator

Choose a reason for hiding this comment

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

@jcabak - by removing this link you're reverting #532. Was this intentional?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Investigating further, it seems that this was by mistake, but I'd appreciate a confirmation @jcabak.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I've created #694 to track this issue.

@LisaFC
Copy link
Collaborator

LisaFC commented Sep 20, 2021

Thanks for noticing this @chalin! Waiting to hear from @jcabak about the removed CSS, it does look like an error (possibly by editing an old version of the file?) to me.

@chalin
Copy link
Collaborator

chalin commented Oct 6, 2021

@jcabak - any updates?

@chalin
Copy link
Collaborator

chalin commented Oct 7, 2021

Sorry @chalin, I don't know why I made mistakes, especially with removal stylesheet for tabbed pane and d-print-none. It shouldn't be like that.

No worries. As I mentioned to @LisaFC elsewhere, me might need to discuss & revisit the PR submission and review process to avoid such unintended consequences (e.g., by using squash-merges and requiring that PRs be submitted against the most recent HEAD).

Thanks for confirming! Now we can move forward with fixes.

@LisaFC
Copy link
Collaborator

LisaFC commented Oct 7, 2021

Thanks so much @chalin for raising this and looking in to what happened!

And agree, we should look at what we can do to ensure that PRs aren't against an outdated version of the theme, particularly if they're large/complex. @emckean, did you have some/any ideas?

@emckean
Copy link
Collaborator

emckean commented Oct 7, 2021

yes! I'm working on setting up some visual regression testing against docsy-example (with Percy: emckean/docsy-example#2), so that we can see more easily when something breaks. But setting some PR rules as @chalin suggested is a great idea! I have some time blocked for this tomorrow, let me see if I can push this to the top of the list ...

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.

None yet

4 participants