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

Increase z-index for .show-on-focus #2168

Merged
merged 3 commits into from
Jul 25, 2022
Merged

Conversation

owenniblock
Copy link
Contributor

@owenniblock owenniblock commented Jul 22, 2022

What are you trying to accomplish?

When focused on the "Skip to content" button and not logged in, the button remains hidden.

Before:
The GitHub header with no skip link visible

After
The GitHub header with skip link visible

What approach did you choose and why?

I increased the z-index to make it visible.

What should reviewers focus on?

All good 👍

Can these changes ship as is?

  • Yes, this PR does not depend on additional changes. 🚢

@owenniblock owenniblock requested a review from a team as a code owner July 22, 2022 11:54
@changeset-bot
Copy link

changeset-bot bot commented Jul 22, 2022

🦋 Changeset detected

Latest commit: 8a8c3ab

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@primer/css Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@github-actions github-actions bot temporarily deployed to Storybook Preview July 22, 2022 12:02 Inactive
@owenniblock owenniblock temporarily deployed to github-pages July 22, 2022 12:07 Inactive
@github-actions github-actions bot temporarily deployed to Storybook Preview July 22, 2022 12:08 Inactive
Copy link
Contributor

@simurai simurai left a comment

Choose a reason for hiding this comment

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

It's always hard to tell if changing z-index will cause any side effects? 😅 But maybe fine for this case, considering the intention of .show-on-focus. Also the new AppFrame component sets that "Skip to content" button to 1000.

@github-actions github-actions bot temporarily deployed to Storybook Preview July 25, 2022 08:23 Inactive
@keithamus keithamus merged commit a52afe7 into main Jul 25, 2022
@keithamus keithamus deleted the increase-z-index-for-show-on-focus branch July 25, 2022 08:39
@primer-css primer-css mentioned this pull request Jul 25, 2022
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.

3 participants