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

Make notices push down content #13614

Merged
merged 3 commits into from Feb 14, 2019

Conversation

@jasmussen
Copy link
Contributor

jasmussen commented Jan 31, 2019

This PR restores the good stuff from #12301. That is: it allows notices to push down content. Dismissible notices are sticky at the top, non-dismisible notices scroll out of view.

This is mostly an exact copy of the other PR, but fresh. The behavior has a number of benefits:

  • If you have multiple non-dismissible notices, you can still actually use the editor.
  • Notices no longer cover the scrollbar.
  • Notices no longer cover the permalink interface.
  • Notices now only cover content if you do not dismiss the notices.

Fixes #7276.

GIFs:

desktop

mobile

screenshot 2019-01-31 at 11 58 01

@jasmussen jasmussen self-assigned this Jan 31, 2019

@jasmussen jasmussen requested review from youknowriad , aduth and WordPress/gutenberg-core Jan 31, 2019

@jasmussen

This comment has been minimized.

Copy link
Contributor Author

jasmussen commented Jan 31, 2019

Please read also review points on #12301, as some of them still apply.

@jasmussen

This comment has been minimized.

Copy link
Contributor Author

jasmussen commented Jan 31, 2019

This also mitigates the situation in https://core.trac.wordpress.org/ticket/46098#comment:6.

@karmatosed karmatosed added this to Tighten Up in Phase 2 Jan 31, 2019

@mapk

mapk approved these changes Feb 5, 2019

Copy link
Contributor

mapk left a comment

This worked great for me. Both dismissible and non-dismissible notices performed as expected. The scrolling of content behind dismissible notices, but not behind non-dismissible notices was a good experience.

@jasmussen jasmussen added this to the 5.1 (Gutenberg) milestone Feb 5, 2019

@jasmussen jasmussen requested a review from WordPress/gutenberg-core Feb 5, 2019

@jasmussen

This comment has been minimized.

Copy link
Contributor Author

jasmussen commented Feb 5, 2019

Nice, thank you. I put this in a milestone and expanded the review range for a code review.

@gziolo gziolo requested a review from noisysocks Feb 5, 2019

@noisysocks noisysocks removed this from Tighten Up in Phase 2 Feb 7, 2019

@jasmussen jasmussen added this to Tighten Up in Phase 2 Feb 8, 2019

@aduth

This comment has been minimized.

Copy link
Member

aduth commented Feb 8, 2019

In Firefox, I see an issue at the medium range viewport size (~970px).

Using: [ ...Array( 3 ) ].forEach( () => wp.data.dispatch( 'core/notices' ).createInfoNotice( 'Not dismissible', { isDismissible: false } ) )

image

Make notices push down content
This PR restores the good stuff from #12301. That is: it allows notices to push down content. Dismissible notices are sticky at the top, non-dismisible notices scroll out of view.

This is mostly an exact copy of the other PR, but fresh. The behavior has a number of benefits:

- If you have multiple non-dismissible notices, you can still actually use the editor.
- Notices no longer cover the scrollbar.
- Notices no longer cover the permalink interface.
- Notices now only cover content if you do not dismiss the notices.

@jasmussen jasmussen force-pushed the try/restore-notices-push-content branch from 9deb5de to 33ddecf Feb 12, 2019

@jasmussen

This comment has been minimized.

Copy link
Contributor Author

jasmussen commented Feb 12, 2019

Noticed an issue with this branch that isn't fixed:

cover sticky

Notice how the sticky notice covers the block toolbar.

However, compare to master:

master

It's even worse there.

For that reason, I feel like it would still be good to ship this as an improvement.

@jasmussen

This comment has been minimized.

Copy link
Contributor Author

jasmussen commented Feb 12, 2019

Looking into the issue, thanks Andrew. It's not Firefox specific, it's because of the top toolbar.

@jasmussen

This comment has been minimized.

Copy link
Contributor Author

jasmussen commented Feb 12, 2019

Solid catch, Andrew. Turns out there was a lot of old cruft there that was vestigial and could be killed entirely. So the most recent commit should fix issues even in master:

breakpoints

I think this should be good for a final review.

jasmussen added a commit that referenced this pull request Feb 12, 2019

Try: Smoother animation for the main content area.
This PR tries to add additional animation when the sidebar open/closes. Specifically, it now animates the right-margin in lockstep with the sidebar opening and closing. This causes the main body content to animate, just like the sidebar, for a nicer experience.

However in order to be performant, this means we add `will-change: transform;` to the .edit-post__layout-content`. This is causing an issue with the notices, which will be moot when #13614 gets merged, so *this PR is dependant on that landing first*.

Secondly, although it's a small PR, the addition of the will-change property is likely going to need a lot of testing so it doesn't cause regressions. Things to look for are absolute, fixed, or sticky position elements and the like. In my own testing, only the notices regressed which as mentioned gets fixed when 13614 lands. But this will still need a lot of eyes.
@aduth

aduth approved these changes Feb 12, 2019

Copy link
Member

aduth left a comment

This works great across all variations I could think to throw at it (dismissible, viewport size, editor sidebar toggle, admin sidebar collapse, scrollable content). 👍

}

// Non-dismissible notices.
&.is-pinned.is-pinned {

This comment has been minimized.

@aduth

aduth Feb 12, 2019

Member

Do we need the .is-pinned to be duplicated here (specificity)? It'd be preferable if we didn't, or could find another way to override the intended "base" styles.

This comment has been minimized.

@jasmussen

jasmussen Feb 13, 2019

Author Contributor

I could swear it was needed, but perhaps it was needed earlier on in the development process, and as things were cleaned up it became unnecessary. In any case I tried removing the extra specificity (yes, cool new trick I learned) and it doesn't seem to have any negative effects.

@kjellr

kjellr approved these changes Feb 12, 2019

Copy link
Contributor

kjellr left a comment

Looks good to me too. Tested with a variety of different notifications and editor scenarios. 👍

@jasmussen

This comment has been minimized.

Copy link
Contributor Author

jasmussen commented Feb 13, 2019

Thanks all for the reviews, I think this is good to go when the checks pass.

@jasmussen jasmussen merged commit 6991141 into master Feb 14, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

Phase 2 automation moved this from Tighten Up to Done Feb 14, 2019

@jasmussen jasmussen deleted the try/restore-notices-push-content branch Feb 14, 2019

@irishetcher

This comment has been minimized.

Copy link

irishetcher commented Feb 18, 2019

That would be an improvement. It's a bit jarring to have the title covered. Thanks for letting me know.

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