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

Sticky panel: add event listener for resizing to update measurements. #396

Merged
merged 2 commits into from
Nov 23, 2015

Conversation

mtias
Copy link
Member

@mtias mtias commented Nov 21, 2015

Also removes sticky state when mobile viewport is detected, otherwise components like EditorGroundControl could get stuck in an incorrect state. (Obscuring the UI.)

Testing

  • Scroll a bit on the editor window to trigger the sticky behaviour; resize window and see that the component always fits.
  • Scroll a bit on the editor, now resize to the smallest window possible (to trigger isMobile checks), then scroll up in mobile view, then resize back to regular window size.

@jasmussen do you mind taking a look at this?

Also removes sticky state when mobile viewport is detected,
otherwise components like EditorGroundControl could get stuck
in an incorrect state. (Obscuring the UI.)
@mtias mtias added [Feature] Post/Page Editor The editor for editing posts and pages. [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. Components labels Nov 21, 2015
@jasmussen
Copy link
Member

Haven't reviewed the code, but this fixes the issue I experienced! Nice work! 👍

updateIsSticky: function() {
var isSticky = window.pageYOffset > this.threshold;

if ( viewport.isMobile() ) {
return;
return this.setState( { isSticky: false } );
Copy link
Member

Choose a reason for hiding this comment

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

Do we need to set the state if the stickiness didn’t change?

Copy link
Member Author

Choose a reason for hiding this comment

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

We could definitely include a this.state.isSticky check in the if.

@aduth aduth added [Status] Needs Author Reply and removed [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Nov 22, 2015
@mtias mtias force-pushed the fix/sticky-panel-update-routine branch from 362d979 to 65f0c84 Compare November 23, 2015 12:16
@mtias mtias force-pushed the fix/sticky-panel-update-routine branch from 65f0c84 to 73029cc Compare November 23, 2015 12:19
@mtias mtias added [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. and removed [Status] Needs Author Reply labels Nov 23, 2015
@aduth
Copy link
Contributor

aduth commented Nov 23, 2015

The changes here look and work well 👍

I am noticing that we have similar issues with resize handling of the pinned editor toolbar. This can be addressed separately.

image

@aduth aduth added [Status] Ready to Merge and removed [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Nov 23, 2015
@mtias
Copy link
Member Author

mtias commented Nov 23, 2015

Thanks for the feedback. We can keep tweaking it after it's in, but it's nice to get something basic there to prevent the main issue of "hidden All Posts link".

mtias added a commit that referenced this pull request Nov 23, 2015
Sticky panel: add event listener for resizing to update measurements.
@mtias mtias merged commit 3151deb into master Nov 23, 2015
@mtias mtias deleted the fix/sticky-panel-update-routine branch November 23, 2015 14:09
@mkaz mkaz restored the fix/sticky-panel-update-routine branch November 23, 2015 14:52
@apeatling apeatling deleted the fix/sticky-panel-update-routine branch November 30, 2015 06:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Components [Feature] Post/Page Editor The editor for editing posts and pages.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants