Skip to content

Conversation

danrosenthal
Copy link

@danrosenthal danrosenthal commented May 14, 2019

While tophatting a change in web to replace the web implementation of Sheet with the Polaris implementation, I saw that the breakpoint in the JavaScript is wrong, as navigation should be displayed inclusive of 768px, but not 769px. This results in a sheet which, for one pixel at 769px wide appears as the desktop sheet but functions as the mobile sheet (closing towards the bottom). This fixes that issue.

@danrosenthal danrosenthal added the 🤖Skip Changelog Causes CI to ignore changelog update check. label May 14, 2019
@BPScott BPScott temporarily deployed to polaris-react-pr-1475 May 14, 2019 19:02 Inactive
Copy link
Contributor

@yourpalsonja yourpalsonja left a comment

Choose a reason for hiding this comment

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

Sweet change, Dan!

super mario

@danrosenthal
Copy link
Author

cc: @keyfer @asmockler 🤦‍♂

@danrosenthal
Copy link
Author

I think this is actually wrong. Hang tight

@danrosenthal danrosenthal changed the title [Sheet] fix breakpoint off by 1 [Breakpoints] fix breakpoint off by 1 May 14, 2019
@danrosenthal danrosenthal force-pushed the fix-sheet-responsive branch from aba39aa to 1fc0951 Compare May 14, 2019 20:00
@danrosenthal
Copy link
Author

The JavaScript is actually what needed to be changed here. This has been broken for a long time and we just weren't noticing.

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.

🎩

@danrosenthal danrosenthal removed the 🤖Skip Changelog Causes CI to ignore changelog update check. label May 14, 2019
@danrosenthal danrosenthal force-pushed the fix-sheet-responsive branch from 1fc0951 to ae3ba18 Compare May 14, 2019 20:16
@BPScott BPScott requested a deployment to polaris-react-pr-1475 May 14, 2019 20:16 Abandoned
@danrosenthal danrosenthal merged commit 4282cb2 into master May 14, 2019
@danrosenthal danrosenthal deleted the fix-sheet-responsive branch May 14, 2019 20:21
@danrosenthal danrosenthal temporarily deployed to production May 22, 2019 16:25 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.

4 participants