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

[Navigation] Fix navigation hide on key press #2607

Merged
merged 3 commits into from
Jan 9, 2020

Conversation

kyledurand
Copy link
Member

WHY are these changes introduced?

There's an issue in production where if any item within the Navigation component has focus and you hit the escape key, the nav will try to hide itself off to the left.
Screencast

There's no github issue for this. I randomly stumbled upon it while working on something else.

WHAT is this pull request doing?

This pr adds a check to make sure the nav is both active and on a small screen before calling dismiss.

🎩

  1. Visit the details page in storybook
  2. Click on a nav item
  3. While that item is still active, hit escape. Nothing should happen

How to 🎩

🖥 Local development instructions
🗒 General tophatting guidelines
📄 Changelog guidelines

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

export function Playground() {
  return (
    <Page title="Playground">
      {/* Add the code you want to test in here */}
    </Page>
  );
}

🎩 checklist

  • Tested on mobile
  • Tested on multiple browsers
  • Tested for accessibility
  • Updated the component's README.md with documentation changes
  • Tophatted documentation changes in the style guide
  • For visual design changes, pinged one of @ HYPD, @ mirualves, @ sarahill, or @ ry5n to update the Polaris UI kit

@kyledurand kyledurand force-pushed the nav/fix-keypress-bug branch from da4d402 to 5dee996 Compare January 8, 2020 20:57
@github-actions
Copy link
Contributor

github-actions bot commented Jan 8, 2020

💦 Potential splash zone of changes introduced to src/**/*.tsx in this pull request:

Files modified3
Files potentially affected1

Details

All files potentially affected (total: 1)
📄 UNRELEASED.md (total: 0)

Files potentially affected (total: 0)

🧩 src/components/Frame/Frame.tsx (total: 1)

Files potentially affected (total: 1)

🧩 src/components/Frame/tests/Frame.test.tsx (total: 0)

Files potentially affected (total: 0)


This comment automatically updates as changes are made to this pull request.
Feedback, troubleshooting: open an issue or reach out on Slack in #polaris-tooling.

@kyledurand
Copy link
Member Author

I'm just on a call right now but I'll add a test before the end of the day

@kyledurand kyledurand force-pushed the nav/fix-keypress-bug branch 2 times, most recently from a2ad90a to 5b51e61 Compare January 8, 2020 22:23
@kyledurand kyledurand force-pushed the nav/fix-keypress-bug branch from b8cb9d9 to 97bb2fa Compare January 8, 2020 22:28
Copy link
Contributor

@tmlayton tmlayton left a comment

Choose a reason for hiding this comment

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

Nitpick about the casting, otherwise looks great 👍

Copy link
Member

@AndrewMusgrave AndrewMusgrave left a comment

Choose a reason for hiding this comment

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

Nice catch ✅ 🎩

Co-Authored-By: Andrew Musgrave <andrew.musgrave@shopify.com>
@kyledurand kyledurand force-pushed the nav/fix-keypress-bug branch 2 times, most recently from c89d7e6 to dfec046 Compare January 9, 2020 00:24
Co-Authored-By: Andrew Musgrave <andrew.musgrave@shopify.com>
@kyledurand kyledurand force-pushed the nav/fix-keypress-bug branch from dfec046 to e5a0077 Compare January 9, 2020 00:30
@kyledurand kyledurand merged commit 46ceed5 into master Jan 9, 2020
@kyledurand kyledurand deleted the nav/fix-keypress-bug branch January 9, 2020 14:39
kyledurand added a commit that referenced this pull request Jan 14, 2020
* Fix nav hide bug

* Update src/components/Frame/tests/Frame.test.tsx

Co-Authored-By: Andrew Musgrave <andrew.musgrave@shopify.com>

* Update src/components/Frame/tests/Frame.test.tsx

Co-Authored-By: Andrew Musgrave <andrew.musgrave@shopify.com>

Co-authored-by: Andrew Musgrave <andrew.musgrave@shopify.com>
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.

3 participants