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

Fix the snackbar notices position #18683

Merged
merged 1 commit into from
Nov 22, 2019
Merged

Fix the snackbar notices position #18683

merged 1 commit into from
Nov 22, 2019

Conversation

youknowriad
Copy link
Contributor

This PR fixes another small regression caused by #18044 where the snackbar notices were positioned at the top instead of the bottom left of the page.

Basically the edit-post-layout__content className has been removed. Fortunately, this is the only place it was used. I'm also removing some useless styles I found.

I'm also adding a "Needs dev note" label to document this change in the next WordPress release.

@youknowriad youknowriad added [Type] Bug An existing feature does not function as intended General Interface Parts of the UI which don't fall neatly under other labels. [Type] Regression Related to a regression in the latest release labels Nov 22, 2019
@youknowriad youknowriad requested a review from talldan as a code owner November 22, 2019 07:41
@youknowriad youknowriad self-assigned this Nov 22, 2019
@youknowriad youknowriad requested a review from a team November 22, 2019 07:41
@youknowriad youknowriad added the Good First Review A PR that's suitable for someone looking to contribute for the first time by reviewing code label Nov 22, 2019
}

// 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

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.

@youknowriad youknowriad added the Needs Dev Note Requires a developer note for a major WordPress release cycle label Nov 22, 2019
Copy link
Contributor

@gwwar gwwar left a comment

Choose a reason for hiding this comment

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

Thanks @youknowriad I verified that this fixes the snackbar position. Maybe let's drop changes to edit-post-layout__scrollable-container since I didn't see how this was related in testing. If it's not actually needed, lets split that out to another janitorial.

Screen Shot 2019-11-22 at 10 23 45 AM

@youknowriad youknowriad merged commit a8b6578 into master Nov 22, 2019
@youknowriad youknowriad deleted the fix/notices-position branch November 22, 2019 18:45
@youknowriad youknowriad added this to the Gutenberg 7.0 milestone Nov 25, 2019
@jorgefilipecosta jorgefilipecosta mentioned this pull request Feb 12, 2020
23 tasks
@youknowriad youknowriad removed the Needs Dev Note Requires a developer note for a major WordPress release cycle label Apr 8, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
General Interface Parts of the UI which don't fall neatly under other labels. Good First Review A PR that's suitable for someone looking to contribute for the first time by reviewing code [Type] Bug An existing feature does not function as intended [Type] Regression Related to a regression in the latest release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants