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.

Sign up for GitHub

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

Fix the snackbar notices position #18683

Merged
merged 1 commit into from
Nov 22, 2019
Merged

Fix the snackbar notices position #18683

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 2 additions & 29 deletions packages/edit-post/src/components/layout/style.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,14 +13,14 @@
}

// Adjust the position of the notices
.edit-post-layout__content .components-editor-notices__snackbar {
.edit-post-layout .components-editor-notices__snackbar {
position: fixed;
right: 0;
bottom: 20px;
padding-left: 16px;
padding-right: 16px;
}
@include editor-left(".edit-post-layout__content .components-editor-notices__snackbar");
@include editor-left(".edit-post-layout.components-editor-notices__snackbar");

.edit-post-layout .editor-post-publish-panel {
position: fixed;
Expand Down Expand Up @@ -110,30 +110,3 @@
font-size: $default-font-size;
}
}

.edit-post-layout__scrollable-container {
// On mobile the main content (html or body) area has to scroll.
youknowriad marked this conversation as resolved.
Show resolved Hide resolved
// If, like we do on the desktop, scroll an element (.edit-post-layout__content) you can invoke
// the overscroll bounce on the non-scrolling container, causing for a frustrating scrolling experience.
// The following rule enables this scrolling beyond the mobile breakpoint, because on the desktop
// breakpoints scrolling an isolated element helps avoid scroll bleed.
@include break-small() {
overflow-y: auto;
}
-webkit-overflow-scrolling: touch;

// This rule ensures that if you've scrolled to the end of a container,
// then pause, then keep scrolling downwards, the browser doesn't try to scroll
// the parent element, usually invoking a "bounce" effect and then preventing you
// from scrolling upwards until you pause again.
// This is only necessary beyond the small breakpoint because that's when the scroll container changes.
@include break-small() {
overscroll-behavior-y: none;
}

// Pad the scroll box so content on the bottom can be scrolled up.
padding-bottom: 50vh;
Copy link
Member

Choose a reason for hiding this comment

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

Why is this removed?

Copy link
Member

Choose a reason for hiding this comment

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

I believe this part in necessary for the typewriter experience.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I guess we should restore it then, but it's not clear how it's related since it's very detached from the typewriter experience. Maybe it should be added in JS or in the typewriter component.

Also it's a separate regression from this PR I think.

Copy link
Member

Choose a reason for hiding this comment

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

It's not strictly needed for the typewriter experience. It just makes it nicer. It's styling and it can be adjusted. The comment says what it is: extra space on the bottom of the page so it can be scrolled up more. I'm not sure what's a better place for it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe still this file then by overriding the editor regions style, EditorRegions is meant to be something generic that will be used outside the editor (widgets, ... any page that has the same global layout than the editor) so I don't think it makes sense there.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ellatrix would you min adding the fix to this PR or a separate one. I'm personally not sure what impact this have and to be honest, the type writer experience feels good even without it for me.

Copy link
Contributor

@gwwar gwwar Nov 22, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

I don't really see any difference with these rules added/removed, but I'm also not sure when this class is added. Querying the DOM in the post editor returns 0 nodes for me. Overall reading this, removal of these rules feel unrelated to the fix here. Is this blocking @ellatrix ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

These rules don't do anything at the moment because that class don't exist in the DOM. i think the fix should be done separately as this is just dead code for now. And the fix is not to bring the whole style back but just the padding bottom if I understand properly.

@include break-small {
padding-bottom: 0;
}
}