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

[Frame] Skip to content target / ref #2080

Merged
merged 1 commit into from
Sep 11, 2019
Merged

Conversation

philmongeon
Copy link
Member

@philmongeon philmongeon commented Sep 5, 2019

WHY are these changes introduced?

I discovered a problem working on a project for experts.shopify.com.

We were planning on using a <Toast /> component for some user feedback but the <Toast /> component relies on the <Frame /> component to function.

The <Frame /> component adds a skip to content link for a11y purposes that is not configurable and does not work the way we need it to work.

We already have a skip to content link of our own that places the user past our main navigation to the top of the content area. When we add the frame component to the app we end up with 2 skip to content links (ours that does the thing we want and the one created by the frame that does not function correctly).

Making the skip to content link configurable will allow us to use the Polaris components that rely on the frame component while still providing an accessible skip to content link.

WHAT is this pull request doing?

Adds a new prop to the <Frame /> component.

/** The RefObject you wish to be focused when clicking the skip to content link */
skipToContentTarget?: React.RefObject<HTMLAnchorElement>;

We add a RefObject so that on click we can focus the provided RefObject and focus the user in the correct place.

[GIF] Skipping to the target element Description of what the gif shows

How to 🎩

🖥 Local development instructions
🗒 General tophatting guidelines
📄 Changelog guidelines

Copy-paste this code in playground/Playground.tsx:
import React from 'react';
import {Page, Frame} from '../src';

export function Playground() {
  const targetId = 'SkipToContentTarget';
  const targetRef = React.createRef<HTMLAnchorElement>();

  const skipToMainContentTarget = (
    // eslint-disable-next-line jsx-a11y/anchor-is-valid
    <a id={targetId} ref={targetRef} tabIndex={-1}>
      Focus target
    </a>
  );

  return (
    <Page title="Playground">
      <Frame skipToContentTargetId={targetId} skipToContentTarget={targetRef}>
        {skipToMainContentTarget}
      </Frame>
    </Page>
  );
}

🎩 checklist

@ghost
Copy link

ghost commented Sep 5, 2019

👋 Thanks for opening your first pull request. A contributor should give feedback soon. If you haven’t already, please check out the contributing guidelines.

@dleroux
Copy link
Contributor

dleroux commented Sep 5, 2019

🎩 works great! I like that this was brought down to 1 prop. Changelog needs an update but apart from that ✅ . Ping me when the that is ready. Thank you for doing this!

@philmongeon philmongeon force-pushed the skip-to-content-target branch 4 times, most recently from 3c3bb3a to e96f360 Compare September 5, 2019 19:56
@philmongeon
Copy link
Member Author

This works but not in the way I would expect. A bit or weirdness is happening with passing the ref and using it as a ternary to render things. I’ll take a look at fixing it when I have some more time and I’ll ping you then. Thanks !

@philmongeon philmongeon changed the title [Frame] Skip to content target / ref WIP [Frame] Skip to content target / ref Sep 5, 2019
@philmongeon philmongeon changed the title WIP [Frame] Skip to content target / ref [Frame] Skip to content target / ref Sep 9, 2019
@philmongeon
Copy link
Member Author

@dleroux updated and added changes to readme and 🎩 the styleguide

@alex-page
Copy link
Member

I tophatted this in chroma -> frame in mac os, firefox nightly.

Copy link
Contributor

@dleroux dleroux left a comment

Choose a reason for hiding this comment

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

Thanks for doing this! 🎩 and works great! 🐑 🇮🇹

@philmongeon philmongeon merged commit 2474679 into master Sep 11, 2019
@philmongeon philmongeon deleted the skip-to-content-target branch September 11, 2019 13:41
@ghost
Copy link

ghost commented Sep 11, 2019

🎉 Thanks for your contribution to Polaris React!

@dleroux dleroux temporarily deployed to production September 20, 2019 17:44 Inactive
@dleroux dleroux temporarily deployed to production September 23, 2019 15:28 Inactive
@dleroux dleroux temporarily deployed to production September 23, 2019 15:38 Inactive
@dleroux dleroux temporarily deployed to production September 23, 2019 16:59 Inactive
@dleroux dleroux temporarily deployed to production September 23, 2019 17:05 Inactive
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

3 participants