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 sidebar jumpiness. #20355

Merged
merged 1 commit into from Feb 21, 2020
Merged

Fix sidebar jumpiness. #20355

merged 1 commit into from Feb 21, 2020

Conversation

@jasmussen
Copy link
Contributor

jasmussen commented Feb 21, 2020

The sidebar grows wider when the scrollbar appears. This is a regression compared to WordPress 5.4, where the scrollbar instead indented the content. This PR fixes that.

Before:

before

After:

after

Just to be clear, this only restores the behavior that has shipped with WordPress for a while already. You can verify this by testing WordPress 5.4 without the plugin.

@jasmussen jasmussen requested a review from youknowriad Feb 21, 2020
@jasmussen jasmussen requested a review from ellatrix as a code owner Feb 21, 2020
@jasmussen jasmussen self-assigned this Feb 21, 2020
@github-actions

This comment has been minimized.

Copy link

github-actions bot commented Feb 21, 2020

Size Change: +27 B (0%)

Total Size: 864 kB

Filename Size Change
build/edit-post/style-rtl.css 8.7 kB +14 B (0%)
build/edit-post/style.css 8.69 kB +13 B (0%)
ℹ️ View Unchanged
Filename Size Change
build/a11y/index.js 1.01 kB 0 B
build/annotations/index.js 3.43 kB 0 B
build/api-fetch/index.js 3.39 kB 0 B
build/autop/index.js 2.58 kB 0 B
build/blob/index.js 620 B 0 B
build/block-directory/index.js 6.02 kB 0 B
build/block-directory/style-rtl.css 760 B 0 B
build/block-directory/style.css 760 B 0 B
build/block-editor/index.js 104 kB 0 B
build/block-editor/style-rtl.css 9.78 kB 0 B
build/block-editor/style.css 9.77 kB 0 B
build/block-library/editor-rtl.css 7.67 kB 0 B
build/block-library/editor.css 7.67 kB 0 B
build/block-library/index.js 114 kB 0 B
build/block-library/style-rtl.css 7.47 kB 0 B
build/block-library/style.css 7.48 kB 0 B
build/block-library/theme-rtl.css 669 B 0 B
build/block-library/theme.css 671 B 0 B
build/block-serialization-default-parser/index.js 1.65 kB 0 B
build/block-serialization-spec-parser/index.js 3.1 kB 0 B
build/blocks/index.js 57.6 kB 0 B
build/components/index.js 190 kB 0 B
build/components/style-rtl.css 16.1 kB 0 B
build/components/style.css 16 kB 0 B
build/compose/index.js 5.76 kB 0 B
build/core-data/index.js 10.5 kB 0 B
build/data-controls/index.js 1.04 kB 0 B
build/data/index.js 8.22 kB 0 B
build/date/index.js 5.36 kB 0 B
build/deprecated/index.js 771 B 0 B
build/dom-ready/index.js 569 B 0 B
build/dom/index.js 3.06 kB 0 B
build/edit-post/index.js 90.7 kB 0 B
build/edit-site/index.js 4.58 kB 0 B
build/edit-site/style-rtl.css 2.77 kB 0 B
build/edit-site/style.css 2.76 kB 0 B
build/edit-widgets/index.js 4.36 kB 0 B
build/edit-widgets/style-rtl.css 2.8 kB 0 B
build/edit-widgets/style.css 2.79 kB 0 B
build/editor/editor-styles-rtl.css 327 B 0 B
build/editor/editor-styles.css 328 B 0 B
build/editor/index.js 45.1 kB 0 B
build/editor/style-rtl.css 4.13 kB 0 B
build/editor/style.css 4.11 kB 0 B
build/element/index.js 4.45 kB 0 B
build/escape-html/index.js 733 B 0 B
build/format-library/index.js 7.6 kB 0 B
build/format-library/style-rtl.css 500 B 0 B
build/format-library/style.css 501 B 0 B
build/hooks/index.js 1.92 kB 0 B
build/html-entities/index.js 621 B 0 B
build/i18n/index.js 3.45 kB 0 B
build/is-shallow-equal/index.js 711 B 0 B
build/keyboard-shortcuts/index.js 2.3 kB 0 B
build/keycodes/index.js 1.68 kB 0 B
build/list-reusable-blocks/index.js 2.99 kB 0 B
build/list-reusable-blocks/style-rtl.css 215 B 0 B
build/list-reusable-blocks/style.css 216 B 0 B
build/media-utils/index.js 4.85 kB 0 B
build/notices/index.js 1.57 kB 0 B
build/nux/index.js 3.02 kB 0 B
build/nux/style-rtl.css 616 B 0 B
build/nux/style.css 613 B 0 B
build/plugins/index.js 2.54 kB 0 B
build/primitives/index.js 1.49 kB 0 B
build/priority-queue/index.js 878 B 0 B
build/redux-routine/index.js 2.84 kB 0 B
build/rich-text/index.js 14.3 kB 0 B
build/server-side-render/index.js 2.54 kB 0 B
build/shortcode/index.js 1.7 kB 0 B
build/token-list/index.js 1.27 kB 0 B
build/url/index.js 4 kB 0 B
build/viewport/index.js 1.61 kB 0 B
build/warning/index.js 1.14 kB 0 B
build/wordcount/index.js 1.18 kB 0 B

compressed-size-action

@@ -77,6 +77,10 @@ html.block-editor-editor-skeleton__html-container {
.block-editor-editor-skeleton__sidebar {
display: none;

> div {

This comment has been minimized.

Copy link
@youknowriad

youknowriad Feb 21, 2020

Contributor

I'm not certain this is the right place for this style. Because potentially in other EditorSkeleton usage we could have multiple divs showing up one after the other.

Ideally, this style should be in packages/edit-post/src/components/sidebar/style.scss because that div is being created by the withFocusReturn HoC that is added to the Sidebar component. The problem though is there's no way to target this there, it has no className, nothing.

So a middle-ground solution for me is to move this to packages/edit-post/src/components/layout/style.scss because it's edit-post specific.

This comment has been minimized.

Copy link
@jasmussen

jasmussen Feb 21, 2020

Author Contributor

Happy to move it there. Wanted to note that it is almost certainly the addition of this fragment that caused the regression in the first place, and it was then fixed wrong by setting "overflow" to visible on the descendant. We really should be as careful as we can, with adding fragments. Will try to move the CSS now.

@@ -1,3 +1,7 @@
.block-editor-editor-skeleton__sidebar > div {

This comment has been minimized.

Copy link
@jasmussen

jasmussen Feb 21, 2020

Author Contributor

I'm not sure this is actually better. There is NO way around this, short of removing the fragment entirely, or giving it a CSS class to target separately, we have to target this div.

Is this better, @youknowriad?

This comment has been minimized.

Copy link
@youknowriad

youknowriad Feb 21, 2020

Contributor

I think that's better, I have a small preference for this being moved to the layout style but it's fine here.

This comment has been minimized.

Copy link
@jasmussen

jasmussen Feb 21, 2020

Author Contributor

Let me move it to the layout style, that's no trouble. It's not important to me where it lives, just that it's present :)

This comment has been minimized.

Copy link
@jasmussen

jasmussen Feb 21, 2020

Author Contributor

Moved, let me know if it's still okay to go.

@youknowriad

This comment has been minimized.

Copy link
Contributor

youknowriad commented Feb 21, 2020

go go

@jasmussen

This comment has been minimized.

Copy link
Contributor Author

jasmussen commented Feb 21, 2020

Pretty sure this fixes #20206.

@jasmussen

This comment has been minimized.

Copy link
Contributor Author

jasmussen commented Feb 21, 2020

Confirmed, this fixes #20206.

Before:

before

After:

after

The sidebar grows wider when the scrollbar appears. This is a regression compared to WordPress 5.4, where the scrollbar instead indented the content. This PR fixes that.
@jasmussen jasmussen force-pushed the try/scrollbar-fix branch from 97172d2 to 6b7313e Feb 21, 2020
@jasmussen

This comment has been minimized.

Copy link
Contributor Author

jasmussen commented Feb 21, 2020

Squashes, so as to force the tests to run again. I could not restart them from the Travis UI.

@jasmussen jasmussen merged commit a160f76 into master Feb 21, 2020
3 checks passed
3 checks passed
build
Details
pull-request-automation
Details
Travis CI - Pull Request Build Passed
Details
@jasmussen jasmussen added this to Done in WordPress 5.4 Must Have via automation Feb 21, 2020
@jasmussen jasmussen deleted the try/scrollbar-fix branch Feb 21, 2020
@github-actions github-actions bot added this to the Gutenberg 7.6 milestone Feb 21, 2020
@epiqueras

This comment has been minimized.

Copy link
Contributor

epiqueras commented Feb 21, 2020

It's crazy how two lines can make such a difference. This behavior was a pet peeve of mine. Thanks for fixing it!

@jasmussen

This comment has been minimized.

Copy link
Contributor Author

jasmussen commented Feb 21, 2020

Frustrated me as well, no worries.

jorgefilipecosta added a commit that referenced this pull request Feb 24, 2020
The sidebar grows wider when the scrollbar appears. This is a regression compared to WordPress 5.4, where the scrollbar instead indented the content. This PR fixes that.
jorgefilipecosta added a commit that referenced this pull request Feb 24, 2020
The sidebar grows wider when the scrollbar appears. This is a regression compared to WordPress 5.4, where the scrollbar instead indented the content. This PR fixes that.
@u1qqlv

This comment has been minimized.

Copy link

u1qqlv commented Feb 27, 2020

the is a problem is the browser size is too small...
see-problem

jasmussen pushed a commit that referenced this pull request Feb 27, 2020
Fixes feedback from #20355 (comment).
@jasmussen

This comment has been minimized.

Copy link
Contributor Author

jasmussen commented Feb 27, 2020

Thanks for the report, @u1qqlv, I've pushed a fix in #20491.

jasmussen added a commit that referenced this pull request Feb 28, 2020
Fixes feedback from #20355 (comment).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants
You can’t perform that action at this time.