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

feat(SkipToContent): Update skip to content to support react router #4831

Closed

Conversation

jessiehuff
Copy link
Contributor

@jessiehuff jessiehuff commented Sep 16, 2020

What: Closes #4809

@jessiehuff jessiehuff changed the title Feat/update skip to content feat(SkipToContent): Update skip to content to support react router Sep 16, 2020
@patternfly-build
Copy link
Contributor

patternfly-build commented Sep 16, 2020

@jschuler jschuler self-requested a review September 16, 2020 19:00
jschuler
jschuler previously approved these changes Sep 21, 2020
Copy link
Contributor

@tlabaj tlabaj left a comment

Choose a reason for hiding this comment

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

Can you add new prop to demo app please and add an integration test.

Copy link
Contributor

@tlabaj tlabaj left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@redallen redallen left a comment

Choose a reason for hiding this comment

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

I don't see a SkipToContent anywhere in OpenShift, but if there was one my guess is that it has href="/#main-content" instead of href="#main-content" which is causing your undesired behavior.

All react-router does in addition to the default browser behavior for hash links of document.getElementById('main-content').scrollTo() is check to see which route it should render, which should always be the same if not using a HashRouter. In the case of SkipToContent we should always want the browser behavior to take place, not a router's.

I'm happy to help you debug.

@jessiehuff
Copy link
Contributor Author

Sure @redallen! Definitely interested in any insight you might have. This is the PR I had with OpenShift trying to add the Skip to Content. I was attempting to do: href={'#content'}

@redallen
Copy link
Contributor

It doesn't work because of the <base href="/" /> tag that OpenShift includes. Try this instead:

<SkipToContent href={`${this.props.location.pathname}#content`}>Skip to Content</SkipToContent>`

@jschuler
Copy link
Collaborator

@redallen react-router has some issues with hash links afaik. see remix-run/react-router#394 (comment)
I think that is the main reason openshift uses https://www.npmjs.com/package/react-router-hash-link

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update skip to content component to support react router
5 participants