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

Made notices always visible #4130

Merged
merged 3 commits into from
Dec 27, 2017
Merged

Conversation

jorgefilipecosta
Copy link
Member

@jorgefilipecosta jorgefilipecosta commented Dec 21, 2017

Description

This PR aims to fix the visual issue described in #4128 and #3777. It makes the notices always visible even when the post is scrolled.
Besides being fixed sticky behavior has required for things to look as expected in resolutions < 600px.

How Has This Been Tested?

Add a post with much content (requires scroll).
Save the post, resize the window to width < 600px scroll from the top until the end and verify the notice is correctly shown and the sticky behavior looks as expected.
Resize the window to points between this set of number {600, 782, 960, >960}, verify that notice always adapts and is always shown in the correct position even the look changes (elements are hidden shown)

Screenshots (jpeg or gifs if applicable):

screen shot 2017-12-21 at 16 46 35

screen shot 2017-12-21 at 16 46 58

screen shot 2017-12-21 at 16 47 19

screen shot 2017-12-21 at 16 47 40

screen shot 2017-12-21 at 16 47 56

cc: @afercia

@jorgefilipecosta jorgefilipecosta added Needs Design Feedback Needs general design feedback. [Type] Bug An existing feature does not function as intended labels Dec 21, 2017
@jorgefilipecosta jorgefilipecosta self-assigned this Dec 21, 2017
@youknowriad
Copy link
Contributor

Mmm, this is not easy to get right because there's so many use-cases:

For example:

  • Close the Post Settings sidebar
  • Close the WP left sidebar explicitely

screen shot 2017-12-21 at 18 34 24

screen shot 2017-12-21 at 18 33 54

Some of these styles (those concerning the WP left sidebar) are already defined for the .editor-header. Might be good to extract those as a mixin. And we could do another mixin for the right sidebar

@jorgefilipecosta jorgefilipecosta added the [Status] In Progress Tracking issues with work in progress label Dec 21, 2017
The mixins apply editor left/right position to the selector passed as argument. It allows to use position fixed with correct dimensions.
…dths lower than small breakpoint.

Used editor-left & editor-right mixins to correctly position notices.
This change removes duplicate code and solves a bug related with resizing to small screen expanding WordPress menu and then resizing back to big screen.
@jorgefilipecosta jorgefilipecosta force-pushed the fix/notices-always-visible branch from b36767e to 7422474 Compare December 22, 2017 14:48
@jorgefilipecosta
Copy link
Member Author

Hi @youknowriad, thank you for your review. Yes, this more complex than what I expected. I refactored this and created a mixin as you suggested. The mixin uses the logic implemented for .editor-header, but I found an existing bug and I added minor changes to the logic.
The bug can be replicated by resizing the screen to mobile, opening the responsive menu, keeping it open and resize back to the desktop. The width keeps the same as when the responsive menu was opened no matter if the menu is collapsed or not.
Before:
screen shot 2017-12-22 at 14 50 04

After:
screen shot 2017-12-22 at 14 48 40

@jorgefilipecosta jorgefilipecosta removed the [Status] In Progress Tracking issues with work in progress label Dec 22, 2017
/**
* Applies editor left position to the selector passed as argument
*/
@mixin editor-left( $selector ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it possible to avoid the selector parameter and use &? that way, we could embed the @include call instead of passing the selector as a parameter. I don't know if it's possible personally but seems nicer if so.

Copy link
Member Author

Choose a reason for hiding this comment

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

Hi @youknowriad, you mean use something like:
.components-notice-list {
@include editor-left;
}
Unfortunately, I don't think this is possible in our case. Even if we could use & all the styles included would be applied to a descendant of .components-notice-list

@youknowriad
Copy link
Contributor

I really like these mixins 👍

Copy link
Contributor

@youknowriad youknowriad left a comment

Choose a reason for hiding this comment

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

Nice work here

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs Design Feedback Needs general design feedback. [Type] Bug An existing feature does not function as intended
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants