added skip-nav to missing pages #23770

Wu-Bowen · 2020-05-05T01:18:36Z

Description

Added reach-skip-nav IDs to main elements that were missing them.

Documentation

Related Issues

Related to issue #21565

tesseralis

Hi @Wu-Bowen! Thanks for your PR.

As I mentioned in the issue, I think the best way to tackle this is by using the <SkipNavContent> component. Using the ID is flaky — if someone mistypes it, it fails silently and the skip link will not work. Using the component means we get a build time error if the component was included incorrectly.

Wu-Bowen · 2020-05-06T21:30:16Z

Hi @tesseralis,
Sorry about the component part, I must have missed the comment in the original issue. I changed all the SkipNavContent on the aforementioned pages. I had a question on where some of these pages are used. For example, the 404 page and the ecosystem page are easy enough to find, but for the template pages, I added the component, but I do not know where on the website I can go to test if they are working.

tesseralis · 2020-05-07T22:24:20Z

One thing I noticed from doing some research on skip-navs (ref: WebAIM a11y-project) was that they tend to be attached to the <main> tag. But Reach's <SkipNavComponent> only renders a div.

After talking with @madalynrose, I think the best solution is to roll out our own <MainContent> component that attaches the ID prop:

function MainContent({ children, …props }) {
  <main id="reach-skip-nav" {…props}>
    {children}
  </main>
}

And then we can use this component in all our pages, replacing the raw reference to ID.

tesseralis · 2020-05-07T22:31:18Z

As for testing the template pages, check the different files under src/utils/node. These should tell you which pages and urls are associated with which templates. For example, in utils/node/starters:

  starters.forEach((node, index) => {
    createPage({
      path: `/starters${node.fields.starterShowcase.slug}`,
      component: slash(starterTemplate),
      context: {
        slug: node.fields.starterShowcase.slug,
      },
    })
  })

we see that template-starter-page is being used to create pages with the hash /starters/{slug}. So we know it's responsible for making pages like: https://www.gatsbyjs.org/starters/draftbox-co/gatsby-ghost-balsa-starter/

You can also use file search find where different packages are imported. For example, if you search for template-starter-page you should point you to the file in utils/node that uses that template.

Wu-Bowen · 2020-05-10T01:07:27Z

@tesseralis Just to understand, you want us to create a MainContent component that will be imported to all the pages. Then, to use this component instead of the id="reach-skip-nav"?

tesseralis · 2020-05-11T20:54:41Z

@tesseralis Just to understand, you want us to create a MainContent component that will be imported to all the pages. Then, to use this component instead of the id="reach-skip-nav"?

@Wu-Bowen correct!

Wu-Bowen · 2020-05-14T01:32:22Z

Hi @tesseralis, we added a new MainContent component and tested it on the pages that you had listed above. Please let us know if this was what you were looking to do and whether or not we should implement it for the rest of the pages.

tesseralis

Excellent start! Yes, this is what I intended and I think we should convert the rest of the files to use this main tag.

Also, can you be sure to run yarn format? Some of the changes are inconsistent with our formatting.

tesseralis · 2020-05-15T02:06:08Z

www/src/pages/404.js

@@ -2,18 +2,21 @@ import React from "react"
 import Container from "../components/container"
 import Link from "../components/localized-link"
 import FooterLinks from "../components/shared/footer-links"
+import Main from "../components/main-content"

 export default function FourOhFour() {
  return (
    <>


We don't need this fragment any more since everything is wrapped in a <Main> tag.

tesseralis · 2020-05-15T23:04:32Z

@Wu-Bowen out of curiousity, what command did you run to format www? It's picking up some formatting errors that weren't picked up before and I'm worried that our linters might be broken.

Wu-Bowen · 2020-05-16T17:34:23Z

@Wu-Bowen out of curiousity, what command did you run to format www? It's picking up some formatting errors that weren't picked up before and I'm worried that our linters might be broken.

Yea I noticed that, but I didn't know exactly which files were changed, so I couldn't revert it. I believe I ran yarn prettier --write src/**/*.js. Sorry if this was the wrong command, I wasn't too sure what Gatsby's built-in formatter was.

tesseralis · 2020-05-18T20:43:26Z

@Wu-Bowen Ah, I see. It appears that our linter hasn't been working for www for the past couple of months. I created a PR to fix that here: #24133.

tesseralis · 2020-05-18T20:46:10Z

So for now I think we can do one of two things:

wait for that PR to be merged.
undo your formatting changes and try to run prettier on only the files that you are changing.

If you'd rather get unblocked, feel free to do 2. But if you feel that's too much work already and would rather wait, that's fine as well.

tesseralis · 2020-05-19T21:30:00Z

@Wu-Bowen my eslint PR just got through! Could you fix the conflicts and update this branch? Thanks!

tesseralis

Almost there! Just a couple of changes.

tesseralis · 2020-05-28T23:25:43Z

www/src/components/main-content.js

@@ -0,0 +1,11 @@
+import React from "react"
+
+const MainContent = ({ children, ...props }) => {


Prefer

export default function MainContent(…)

We've been inconsistent in the past, but going forward we should give components names: https://twitter.com/dan_abramov/status/1255229440860262400

In addition, could you write a comment that describes what this component is used for? e.g.:

/** * <main> content wrapper that should be included in every page on the site. */

tesseralis · 2020-05-28T23:29:59Z

www/src/pages/404.js

@@ -14,6 +15,6 @@ export default function FourOhFour() {
        </Link>
      </Container>
      <FooterLinks />
-    </>
+    </Main>


<FooterLinks> should not be nested in <main>: https://stackoverflow.com/questions/20470345/should-the-header-and-footer-tags-be-inside-the-main-tag

In general, the components should probably be right next to each other.

LekoArts · 2020-06-22T12:56:23Z

Hi @Wu-Bowen 👋 Do you have time to make the requested changes and solve the merge conflict? :)

freiksenet · 2020-07-03T08:55:41Z

Hi!

I'm going to close this now, as this appears to have been abandoned. Feel free to reopen this PR if you find time to address the comments.

Thanks!

added reach-skip-nav

adac78c

Wu-Bowen requested a review from a team as a code owner May 5, 2020 01:18

gatsbot bot added the status: triage needed Issue or pull request that need to be triaged and assigned to a reviewer label May 5, 2020

Wu-Bowen changed the title ~~added reach-skip-nav~~ added skip-nav to missing pages May 5, 2020

pieh added topic: a11y Related to accessibility topic: website and removed status: triage needed Issue or pull request that need to be triaged and assigned to a reviewer labels May 5, 2020

tesseralis requested review from 0x80 and madalynrose and removed request for 0x80 May 6, 2020 00:51

tesseralis suggested changes May 6, 2020

View reviewed changes

added SkipNavContent component to missing pages

ab030e8

Wu-Bowen requested a review from tesseralis May 6, 2020 21:24

Added MainContent Component

7b0d03a

tesseralis suggested changes May 15, 2020

View reviewed changes

Wu-Bowen added 2 commits May 14, 2020 23:30

formatting changes and added converted some files

bfe7b65

removed fragment

deb1a8b

Merge branch 'master' into master

548b3ce

Wu-Bowen requested a review from tesseralis May 19, 2020 22:17

tesseralis suggested changes May 28, 2020

View reviewed changes

marcysutton added the status: awaiting author response Additional information has been requested from the author label Jun 2, 2020

freiksenet closed this Jul 3, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

added skip-nav to missing pages #23770

added skip-nav to missing pages #23770

Wu-Bowen commented May 5, 2020

tesseralis left a comment

Wu-Bowen commented May 6, 2020

tesseralis commented May 7, 2020

tesseralis commented May 7, 2020

Wu-Bowen commented May 10, 2020

tesseralis commented May 11, 2020

Wu-Bowen commented May 14, 2020

tesseralis left a comment

tesseralis May 15, 2020

tesseralis commented May 15, 2020

Wu-Bowen commented May 16, 2020

tesseralis commented May 18, 2020

tesseralis commented May 18, 2020

tesseralis commented May 19, 2020

tesseralis left a comment

tesseralis May 28, 2020

tesseralis May 28, 2020

tesseralis May 28, 2020

LekoArts commented Jun 22, 2020

freiksenet commented Jul 3, 2020

		@@ -0,0 +1,11 @@
		import React from "react"

		const MainContent = ({ children, ...props }) => {

added skip-nav to missing pages #23770

added skip-nav to missing pages #23770

Conversation

Wu-Bowen commented May 5, 2020

Description

Documentation

Related Issues

tesseralis left a comment

Choose a reason for hiding this comment

Wu-Bowen commented May 6, 2020

tesseralis commented May 7, 2020

tesseralis commented May 7, 2020

Wu-Bowen commented May 10, 2020

tesseralis commented May 11, 2020

Wu-Bowen commented May 14, 2020

tesseralis left a comment

Choose a reason for hiding this comment

tesseralis May 15, 2020

Choose a reason for hiding this comment

tesseralis commented May 15, 2020

Wu-Bowen commented May 16, 2020

tesseralis commented May 18, 2020

tesseralis commented May 18, 2020

tesseralis commented May 19, 2020

tesseralis left a comment

Choose a reason for hiding this comment

tesseralis May 28, 2020

Choose a reason for hiding this comment

tesseralis May 28, 2020

Choose a reason for hiding this comment

tesseralis May 28, 2020

Choose a reason for hiding this comment

LekoArts commented Jun 22, 2020

freiksenet commented Jul 3, 2020