Sidebar: Add a sidebar to hold the post settings and inspector #449
Conversation
youknowriad
added
the
Framework
|
Yaaaaay! I love it! I will definitely give it some love and attention one of these days. Code looks good too. Long term we'll want it shown by default, and its state saved (so if you dismiss it, blog, then come back to the editor, the sidebar is still off until you toggle it back on). We'll want to state-save whether you're using the block editor or text editor too, on that note. Want me to create a separate ticket for this? |
|
Yes, let's create a ticket for that. We may need an API endpoint or use the settings endpoint for these settings. |
editor/components/button/style.scss
Outdated
|
|
||
| &:disabled { | ||
| opacity: 0.6; | ||
| } | ||
|
|
||
| &.is-active { |
There was a problem hiding this comment.
Do we expect this to be the same styling we'll want for the :active pseudo-class ?
If so, should we add it here?
If not, should we be worried about confusion between the two unique states sharing the same name?
There was a problem hiding this comment.
I don't know? :) I've always been confused with the :active state. I think .is-active and :active are different states, maybe we should rename .is-active?
There was a problem hiding this comment.
Maybe a question for @jasmussen for prescribing variations on button state we'll need to support.
There was a problem hiding this comment.
I'm thinking about hover, click and active states these days, across all controls, to ensure consistency. Right now the toggled state is visually the same as :active. Did that answer the question?
There was a problem hiding this comment.
This means all the buttons will have the dark gray background when clicked (before releasing the mouse). I'm not sure we want this, do we?
There was a problem hiding this comment.
I'm sorry, I'm tired and wasn't being very clear indeed.
My mind has been thinking about the hover state, which right now is blue text and a dark gray outline, which I'm not convinced about. I've been thinking whether we could do the inverted version for both hover, active and toggled. I.e. hover would also have the dark gray background. This is in context of the accessibility and contrast discussions that took place a while back. But we might find that this feels too heavy handed, and that we might be able to have a lighter gray hover+active state, in which case only the toggled state would be dark gray.
Bottomline, probably good to not hover/active/toggled states block this PR from being merged, as this is something I will revisit across all buttons later on regardless.
There was a problem hiding this comment.
I think I'm renaming is-active to is-toggled :)
editor/state.js
Outdated
| @@ -148,6 +148,15 @@ export function mode( state = 'visual', action ) { | |||
| return state; | |||
| } | |||
|
|
|||
| export function sidebar( state = { opened: false }, action ) { | |||
There was a problem hiding this comment.
What other properties do you expect this to hold? Wondering if a simpler isSidebarOpened reducer with a boolean value might be more appropriate for now.
There was a problem hiding this comment.
I hesitated, I was expecting we may have a property saying we're showing the PostSettings or the BlockInspector, but yeah. I'll go for isSidebarOpened right now!
There was a problem hiding this comment.
Let's call it PostInspector when the time comes. The idea being that the entire sidebar is the inspector, but when nothing is selected, you are inspecting the post itself, therefore showing the meta boxes.
| .editor-sidebar { | ||
| position: fixed; | ||
| right: 0; | ||
| top: 32px + $header-height; |
There was a problem hiding this comment.
We'll need some breakpoint styling for this. Not sure if we wanted to do that here.
There was a problem hiding this comment.
I'll leave this for a more "designy" eyes :)
There was a problem hiding this comment.
I'll definitely tackle this separately, it's already on my to-do 😉
editor/layout/style.scss
Outdated
| .editor-layout.sidebar-opened .editor-layout__content { | ||
| margin-right: $sidebar-width; | ||
|
|
||
| .editor-text-editor__formatting { |
There was a problem hiding this comment.
It needs to be nested under .editor-layout.sidebar-opened, I can rewrite it :)
There was a problem hiding this comment.
We could also define this in modes/text-editor/style.scss as assuming the existence of the ancestor modifier:
.editor-text-editor__formatting {
// ...
.editor-layout.is-sidebar-opened & {
// ...
}
}There was a problem hiding this comment.
I just learned that we can do .editor-layout.is-sidebar-opened & And yes, this is probably better!
editor/layout/index.js
Outdated
| function Layout( { mode } ) { | ||
| function Layout( { mode, sidebarOpened } ) { | ||
| const className = classnames( 'editor-layout', { | ||
| 'sidebar-opened': sidebarOpened |
There was a problem hiding this comment.
As a modifier class, I might expect some boolean-ish prefix to the class name, maybe has-open-sidebar ?
There was a problem hiding this comment.
or maybe is-sidebar-opened to match the reducer name
There was a problem hiding this comment.
or maybe
is-sidebar-openedto match the reducer name
Yep, that works.
|
Ticket opened in #450 |
editor/components/button/index.js
Outdated
| @@ -4,11 +4,12 @@ | |||
| import './style.scss'; | |||
| import classnames from 'classnames'; | |||
|
|
|||
| function Button( { isPrimary, isLarge, className, ...additionalProps } ) { | |||
| function Button( { isPrimary, isLarge, isActive, className, ...additionalProps } ) { | |||
There was a problem hiding this comment.
Did you intend to rename the prop from isActive to isToggled as well?
There was a problem hiding this comment.
Like I said, "too fast and miss the details" :)
This PRs triggers the work on the editor's sidebar. I kept it minimal, just opening and closing an empty sidebar when clicking on "Post Settings".
This probably needs more design ❤️ cc @jasmussen