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

chore(examples): Add @reach/skip-nav example #21107

Closed
wants to merge 20 commits into from

Conversation

madalynrose
Copy link
Contributor

Description

Add an example project showcasing how to leverage @reach/skip-nav to create the ideal navigation focus experience based on research done by @marcysutton and Fable Labs. The updated recommendation was recently added to our blog post on client-side routing in PR #20540. Because this isn't achievable at the framework level, we should show users how to implement it in their own projects.

@madalynrose madalynrose requested review from a team as code owners January 31, 2020 21:53
Copy link
Contributor

@marcysutton marcysutton left a comment

Choose a reason for hiding this comment

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

Dang, I wrote this review the other night and apparently didn't submit it right away. So here are some delayed comments! Let me know if you have any questions.

@@ -0,0 +1,8 @@
{
Copy link
Contributor

Choose a reason for hiding this comment

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

Does the example need this file?

@@ -0,0 +1,6 @@
# Client-only paths
Copy link
Contributor

Choose a reason for hiding this comment

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

You'll want to update this README to reflect what's in the package (and now I know which one was your starting point)! 😛

@@ -0,0 +1,17 @@
module.exports = {
siteMetadata: {
title: `Using @reach/skip-nav`,
Copy link
Contributor

Choose a reason for hiding this comment

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

I feel like there's more to communicate about this example's purpose. How about something like this?

Suggested change
title: `Using @reach/skip-nav`,
title: `Using @reach/skip-nav with client-side routing`,

},
plugins: [
{
resolve: `gatsby-plugin-typography`,
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this example need the typography plugin?

},
// used to generate rewrites for client only paths
// on demo hosted on Netlify
`gatsby-plugin-netlify`,
Copy link
Contributor

Choose a reason for hiding this comment

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

These next two plugins can probably go away too.

@@ -0,0 +1,11 @@
// Implement the Gatsby API “onCreatePage”. This is
// called after every page is created.
exports.onCreatePage = ({ page, actions }) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this file can be excluded.

{
"name": "gatsby-example-using-reach-skip-nav",
"private": true,
"description": "Gatsby example site demoing @reach/skip-nav",
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
"description": "Gatsby example site demoing @reach/skip-nav",
"description": "Gatsby example site demoing @reach/skip-nav and client-side routing",

@@ -0,0 +1,40 @@
import Typography from "typography"
Copy link
Contributor

Choose a reason for hiding this comment

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

If you do end up removing the typography plugin, this file can go away too.

@@ -0,0 +1,8 @@
export const onRouteUpdate = ({ location, prevLocation }) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe add a comment or two explaining what is happening in this file?

@madalynrose
Copy link
Contributor Author

I'm going to close this PR and reopen one using a simpler starter

@madalynrose madalynrose deleted the reach-skip-nav-example branch February 11, 2020 18:01
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.

2 participants