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 panel collapsing in navigation page #21633

Merged
merged 4 commits into from
Apr 16, 2020

Conversation

talldan
Copy link
Contributor

@talldan talldan commented Apr 16, 2020

Description

Currently on the navigation page (at desktop breakpoints), when collapsing a panel, instead of the panel completely collapsing, only the content collapses and the panel itself remains the height of its grid row.

The reason for this is that the Panel components are currently displayed as grid columns, and in the grid the columns are always the same height, so if there's content in the right hand panel, the left hand panel assumes the same height.

This PR fixes things by wrapping the panels in a <div>. These divs become the grid columns allowing the Panels to collapse if needed.

There might be a better way to do this without resorting to extra divs, I'm open to ideas 😄 .

How has this been tested?

  1. Open the Navigation Page and select a menu with lots of content
  2. Collapse one of the panels while leaving the other open
  3. Observe that panel collapses correctly

Screenshots

Before

Screenshot 2020-04-16 at 12 18 47 pm

After

Screenshot 2020-04-16 at 12 19 22 pm

Types of changes

Bug fix (non-breaking change which fixes an issue)

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.
  • I've updated all React Native files affected by any refactorings/renamings in this PR.

@talldan talldan added [Type] Bug An existing feature does not function as intended [Block] Navigation Affects the Navigation Block labels Apr 16, 2020
@talldan talldan self-assigned this Apr 16, 2020
@github-actions
Copy link

github-actions bot commented Apr 16, 2020

Size Change: +98 B (0%)

Total Size: 840 kB

Filename Size Change
build/edit-navigation/index.js 3.54 kB +9 B (0%)
build/edit-navigation/style-rtl.css 475 B +44 B (9%) 🔍
build/edit-navigation/style.css 475 B +45 B (9%) 🔍
ℹ️ View Unchanged
Filename Size Change
build/a11y/index.js 1.02 kB 0 B
build/annotations/index.js 3.62 kB 0 B
build/api-fetch/index.js 4.01 kB 0 B
build/autop/index.js 2.82 kB 0 B
build/blob/index.js 620 B 0 B
build/block-directory/index.js 6.24 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 105 kB 0 B
build/block-editor/style-rtl.css 10.2 kB 0 B
build/block-editor/style.css 10.2 kB 0 B
build/block-library/editor-rtl.css 7.1 kB 0 B
build/block-library/editor.css 7.1 kB 0 B
build/block-library/index.js 112 kB 0 B
build/block-library/style-rtl.css 7.14 kB 0 B
build/block-library/style.css 7.15 kB 0 B
build/block-library/theme-rtl.css 683 B 0 B
build/block-library/theme.css 685 B 0 B
build/block-serialization-default-parser/index.js 1.88 kB 0 B
build/block-serialization-spec-parser/index.js 3.1 kB 0 B
build/blocks/index.js 57.7 kB 0 B
build/components/index.js 198 kB 0 B
build/components/style-rtl.css 16.7 kB 0 B
build/components/style.css 16.7 kB 0 B
build/compose/index.js 6.66 kB 0 B
build/core-data/index.js 11.1 kB 0 B
build/data-controls/index.js 1.25 kB 0 B
build/data/index.js 8.43 kB 0 B
build/date/index.js 5.47 kB 0 B
build/deprecated/index.js 772 B 0 B
build/dom-ready/index.js 569 B 0 B
build/dom/index.js 3.1 kB 0 B
build/edit-post/index.js 27.8 kB 0 B
build/edit-post/style-rtl.css 12.3 kB 0 B
build/edit-post/style.css 12.3 kB 0 B
build/edit-site/index.js 10.4 kB 0 B
build/edit-site/style-rtl.css 5.02 kB 0 B
build/edit-site/style.css 5.02 kB 0 B
build/edit-widgets/index.js 7.53 kB 0 B
build/edit-widgets/style-rtl.css 4.65 kB 0 B
build/edit-widgets/style.css 4.64 kB 0 B
build/editor/editor-styles-rtl.css 428 B 0 B
build/editor/editor-styles.css 431 B 0 B
build/editor/index.js 43.3 kB 0 B
build/editor/style-rtl.css 3.48 kB 0 B
build/editor/style.css 3.47 kB 0 B
build/element/index.js 4.64 kB 0 B
build/escape-html/index.js 733 B 0 B
build/format-library/index.js 7.32 kB 0 B
build/format-library/style-rtl.css 502 B 0 B
build/format-library/style.css 502 B 0 B
build/hooks/index.js 2.13 kB 0 B
build/html-entities/index.js 622 B 0 B
build/i18n/index.js 3.56 kB 0 B
build/is-shallow-equal/index.js 710 B 0 B
build/keyboard-shortcuts/index.js 2.51 kB 0 B
build/keycodes/index.js 1.91 kB 0 B
build/list-reusable-blocks/index.js 3.12 kB 0 B
build/list-reusable-blocks/style-rtl.css 226 B 0 B
build/list-reusable-blocks/style.css 226 B 0 B
build/media-utils/index.js 5.28 kB 0 B
build/notices/index.js 1.79 kB 0 B
build/nux/index.js 3.4 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.67 kB 0 B
build/primitives/index.js 1.49 kB 0 B
build/priority-queue/index.js 788 B 0 B
build/redux-routine/index.js 2.84 kB 0 B
build/rich-text/index.js 14.8 kB 0 B
build/server-side-render/index.js 2.67 kB 0 B
build/shortcode/index.js 1.7 kB 0 B
build/token-list/index.js 1.28 kB 0 B
build/url/index.js 4.02 kB 0 B
build/viewport/index.js 1.84 kB 0 B
build/warning/index.js 1.14 kB 0 B
build/wordcount/index.js 1.17 kB 0 B

compressed-size-action

@tellthemachines
Copy link
Contributor

There might be a better way to do this without resorting to extra divs, I'm open to ideas 😄 .

I'd give align-items: self-start a go (applied to the element that has grid display set). Might not work on IE, but that's probably not a huge issue?

@talldan
Copy link
Contributor Author

talldan commented Apr 16, 2020

@tellthemachines Nice solution! I managed to get it working on IE11 as well:
Screenshot 2020-04-16 at 3 17 37 pm

The only thing missing is the grid gap between the columns, which I don't think is supported in the old IE syntax.

@talldan talldan force-pushed the fix/navigation-page-panel-collapsing branch from d0b412f to 5a769e6 Compare April 16, 2020 07:36
@tellthemachines
Copy link
Contributor

The only thing missing is the grid gap between the columns, which I don't think is supported in the old IE syntax.

Hmm, autoprefixer should be smart enough to handle that. Either we're not on the latest version or perhaps we don't have advanced grid support turned on; either way, sounds like something fixable (in a separate issue).

Copy link
Contributor

@tellthemachines tellthemachines left a comment

Choose a reason for hiding this comment

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

Looks good!


.edit-navigation-menu-editor__navigation-structure-panel {
// IE11 requires the column to be explicity declared.
grid-column: 1;
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks, I always forget about this bit 😅

@talldan talldan merged commit c519e5b into master Apr 16, 2020
@talldan talldan deleted the fix/navigation-page-panel-collapsing branch April 16, 2020 08:04
@github-actions github-actions bot added this to the Gutenberg 8.0 milestone Apr 16, 2020
@talldan
Copy link
Contributor Author

talldan commented Apr 16, 2020

Oops, this accidentally broke things on mobile.

#21638 makes good.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Block] Navigation Affects the Navigation Block [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