Sidebar reacts to screen size and refactor to use interface package #22565
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jorgefilipecosta
added
[Feature] Widgets Screen
|
Size Change: +344 B (0%) Total Size: 1.12 MB
ℹ️ View Unchanged
|
epiqueras
reviewed
May 23, 2020
Comment on lines
32
to
35
| isPostSaveable: select( 'core/editor' ).isEditedPostSaveable(), | ||
| deviceType: select( | ||
| 'core/edit-post' | ||
| ).__experimentalGetPreviewDeviceType(), |
There was a problem hiding this comment.
I don't see these being used.
epiqueras
reviewed
May 23, 2020
| @@ -154,6 +146,7 @@ function Layout() { | |||
| <LocalAutosaveMonitor /> | |||
| <EditPostKeyboardShortcuts /> | |||
| <EditorKeyboardShortcutsRegister /> | |||
| <SettingsSidebar /> | |||
There was a problem hiding this comment.
Why was this moved here?
There was a problem hiding this comment.
Our slot mechanism for complementary areas works with assumption multiple fills are rendered. Only the fill of the active complementary area is shown, but buttons to toggle the complementary area are rendered for all the areas.
Previously the core sidebar fill was only rendered when the sidebar was enabled, and we had a custom made toggle button for the sidebar.
This PR makes sure the core sidebars use the same logic as the plugin sidebars so we need to move SettingsSidebar to a position where it is unconditionoly rendered even if no sidebars are active. Otherwise, when no sidebars are active, we would not render the toggle button for core sidebars.
epiqueras
reviewed
May 23, 2020
Comment on lines
+27
to
+44
| let sidebar = select( 'core/interface' ).getActiveComplementaryArea( | ||
| 'core/edit-post' | ||
| ); | ||
| if ( | ||
| ! [ 'edit-post/document', 'edit-post/block' ].includes( sidebar ) | ||
| ) { | ||
| if ( select( 'core/block-editor' ).getBlockSelectionStart() ) { | ||
| sidebar = 'edit-post/block'; | ||
| } | ||
| sidebar = 'edit-post/document'; | ||
| } |
There was a problem hiding this comment.
This deserves a comment. Why do we now have a fallback instead of returning null?
epiqueras
reviewed
May 23, 2020
| @@ -135,6 +146,7 @@ function Editor( { settings: _settings } ) { | |||
| <Context.Provider value={ context }> | |||
| <FocusReturnProvider> | |||
| <KeyboardShortcuts.Register /> | |||
| <Sidebar /> | |||
There was a problem hiding this comment.
Why is this sidebar out here now and how is it different from the new one below?
There was a problem hiding this comment.
Does this have the fills for the slot below?
There was a problem hiding this comment.
Why is that abstraction necessary?
There was a problem hiding this comment.
A name like SidebarComplementaryAreaFills would make it easier to grasp.
There was a problem hiding this comment.
Similar to https://github.com/WordPress/gutenberg/pull/22565/files#r430034009. We need to unconditionally render the fills even if currently we don't have an active sidebar. I renamed the component to SidebarComplementaryAreaFills 👍
epiqueras
reviewed
May 23, 2020
Comment on lines
19
to
20
| width: calc(100% + 24px); | ||
| } | ||
|
|
||
| .edit-widgets-sidebar { | ||
| z-index: z-index(".edit-widgets-sidebar"); |
epiqueras
reviewed
May 23, 2020
epiqueras
reviewed
May 23, 2020
epiqueras
reviewed
May 23, 2020
epiqueras
reviewed
May 23, 2020
epiqueras
reviewed
May 23, 2020
…-widgets and edit-site; Add auto close on mobile sidebar behaviour interface package
Co-authored-by: Enrique Piqueras <epiqueras@users.noreply.github.com>
Co-authored-by: Enrique Piqueras <epiqueras@users.noreply.github.com>
Co-authored-by: Enrique Piqueras <epiqueras@users.noreply.github.com>
Co-authored-by: Enrique Piqueras <epiqueras@users.noreply.github.com>
jorgefilipecosta
force-pushed
the
add/block-inspector-customizer-blocks-mobile-sidebars-on-edit-site-and-edit-widgets
branch
from
cd29902
to
543e9e2
Compare
jorgefilipecosta
force-pushed
the
add/block-inspector-customizer-blocks-mobile-sidebars-on-edit-site-and-edit-widgets
branch
from
543e9e2
to
e8f0930
Compare
There was a problem hiding this comment.
Hi @epiqueras thank you for the review. Your feedback was applied!
| @@ -135,6 +146,7 @@ function Editor( { settings: _settings } ) { | |||
| <Context.Provider value={ context }> | |||
| <FocusReturnProvider> | |||
| <KeyboardShortcuts.Register /> | |||
| <Sidebar /> | |||
There was a problem hiding this comment.
Similar to https://github.com/WordPress/gutenberg/pull/22565/files#r430034009. We need to unconditionally render the fills even if currently we don't have an active sidebar. I renamed the component to SidebarComplementaryAreaFills 👍
epiqueras
approved these changes
May 25, 2020
packages/edit-post/src/components/sidebar/settings-sidebar/index.js
Outdated
Show resolved
Hide resolved
packages/edit-post/src/components/sidebar/settings-sidebar/index.js
Outdated
Show resolved
Hide resolved
packages/edit-post/src/components/sidebar/settings-sidebar/index.js
Outdated
Show resolved
Hide resolved
packages/edit-post/src/components/sidebar/settings-sidebar/index.js
Outdated
Show resolved
Hide resolved
packages/edit-post/src/components/sidebar/settings-sidebar/index.js
Outdated
Show resolved
Hide resolved
packages/edit-post/src/components/sidebar/settings-sidebar/index.js
Outdated
Show resolved
Hide resolved
…ex.js Co-authored-by: Enrique Piqueras <epiqueras@users.noreply.github.com>
…ex.js Co-authored-by: Enrique Piqueras <epiqueras@users.noreply.github.com>
…ex.js Co-authored-by: Enrique Piqueras <epiqueras@users.noreply.github.com>
…ex.js Co-authored-by: Enrique Piqueras <epiqueras@users.noreply.github.com>
…ex.js Co-authored-by: Enrique Piqueras <epiqueras@users.noreply.github.com>
…ex.js Co-authored-by: Enrique Piqueras <epiqueras@users.noreply.github.com>
Co-authored-by: Enrique Piqueras <epiqueras@users.noreply.github.com>
jorgefilipecosta
force-pushed
the
add/block-inspector-customizer-blocks-mobile-sidebars-on-edit-site-and-edit-widgets
branch
from
b1a359b
to
fa068eb
Compare
jorgefilipecosta
changed the title
jorgefilipecosta
deleted the
add/block-inspector-customizer-blocks-mobile-sidebars-on-edit-site-and-edit-widgets
branch
oandregal
changed the title
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR applies the following changes: