Chrome: Replace the publish dropdown by a sidebar panel by youknowriad · Pull Request #4119 · WordPress/gutenberg

youknowriad · 2017-12-21T10:57:46Z

This PR is the first step towards the updated publish flow #3496 (comment)

In this initial iteration, I'm not updating the current flow, just replacing the current dropdown by a sidebar panel.

Next PRs will focus on:

Adding a Post Publish Flow
Avoid the panel when updating an existing post.

jasmussen · 2017-12-21T11:24:24Z

Yaaaassss! I love this.

Given we're going with a "combined popover/sidebar", is it possible to make it so if the settings sidebar is toggled off, you still "push in the text"? Right now it covers it:

Also — is it possible to add animation at this point? Doesn't have to be in this PR, but structure wise, can the thing animate in from the right?

Thanks so much for working on this!

youknowriad · 2017-12-21T12:52:38Z

@jasmussen Ok so I had to rethink the redux sidebar state to allow multiple sidebars to be able to fix the text margin issue. It should work now.

I also added a small animation, let me know what you think

jasmussen · 2017-12-21T15:49:22Z

I love the recent change. I'd speed up the animation to be double the speed, at most .2s. But 👍 👍 I think this is a great first step.

Next steps could be:

Make this action only for the "Publish", and not for "Update" or "Schedule"
Build an opt-out button in the bottom of the publish box, and in the ellipsis menu.
Change the button to be "Publish..." when the experience is enabled, and "Publish" when it's not.

We need to think about where to put the "Switch to Draft" button when it can't sit in the dropdown, but we'll solve that I'm sure.

jasmussen · 2017-12-21T15:49:59Z

To be clear, those next steps don't have to be in THIS PR, they can be done separately I think.

youknowriad · 2017-12-21T16:13:42Z

I'd speed up the animation to be double the speed, at most .2s.

Tha animation is already .2s, should I update it to 0.1s

jasmussen · 2017-12-21T16:17:36Z

Ah cool. Yeah let's try .1s

atimmer

Looks good to me 👍🏻. One small question about CSS @keyframes.

atimmer · 2017-12-22T13:59:32Z

editor/assets/stylesheets/_animations.scss

+	animation: slide_in_right 0.1s forwards;
+}
+
+@keyframes slide_in_right {


Are keyframes local to a CSS file? If not, would this need a gutenberg_ prefix?

Mmmm good question, I guess this applies to all of our animations. I wonder if there's a way to nest these animations. If not, it makes sense to add a prefix for me.

cc @jasmussen

I do think animations are globally registered, so if registered once it's available everywhere the CSS is loaded.

It'd be nice to either have reusable animations, or well scoped ones.

Ok! I'm inclined to leave this issue to a separate PR as it's global to all animations.

mcsf

Looks great! (Got merged as I was typing this. 😄 )

Tangent: Design-wise, the dropdown chevron will go away, as it no longer reflects the impending animation:

I see, from the parent issue's mockups, that we can adopt Publish… or Publish! instead, without any other visual cue. That's fine with me. However, I'm wondering if we can use what the Customizer did for widgets as inspiration, with the ¼-turn spinning icon:

mcsf · 2017-12-22T14:34:28Z

editor/edit-post/header/index.js

+					/>
+					<IconButton
+						icon="admin-generic"
+						onClick={ () => onToggleSidebar() }


Does the function need to be bound? Perhaps this is an artefact of something else? I'd expect onClick={ onToggleSidebar }.

It needs to be to avoid passing the event as argument to the action creator :)

facepalm — obviously. I mean, you could just do some insane stuff in mapDispatch like toggleSidebar.bind( null, undefined, false ), but that's just idiotic. 😄 The arrow is naturally better. exits slowly

mcsf · 2017-12-22T14:36:14Z

editor/edit-post/header/index.js

+					<PostPreviewButton />
+					<PostPublishPanelToggle
+						isOpen={ isPublishSidebarOpened }
+						onToggle={ () => onToggleSidebar( 'publish' ) }


Minor, but we could extend mapDispatchToProps:

{ onToggleSidebar: toggleSidebar, onTogglePublishSidebar: toggleSidebar.bind( null, 'publish' ), }

and replace the prop here with onToggle={ onTogglePublishSidebar }.

mcsf · 2017-12-22T14:54:57Z

editor/edit-post/layout/index.js

+		'is-sidebar-opened': layoutHasOpenSidebar,
 		'has-fixed-toolbar': hasFixedToolbar,
 	} );
+	const closePublishPanel = () => onToggleSidebar( 'publish', false );


Minor, similar comment as the previous one. Since these bound action creators don't depend on component data, I'd place them in mapDispatchToProps.

mcsf · 2017-12-22T14:58:22Z

editor/store/actions.js

+ * @return {Object}          Action object
 */
-export function toggleSidebar( isMobile ) {
+export function toggleSidebar( sidebar, force ) {


Also minor, but both arguments are nouns, yet they express different things, IMO. sidebar is the object of the action, while force is circumstantial. My suggestion would be shouldForce, in keeping with the convention for booleans. (To be super strict, one could rename the former to sidebarName, but I'm fine with sidebar.)

I think forcedValue is better than shouldForce because it represents the value and not a flag of whether to force or not

mtias · 2017-12-26T16:33:57Z

@mcsf I believe the design plan is that the Publish menu uses an ellipsis instead of a chevron. cc @jasmussen

youknowriad · 2017-12-26T16:47:55Z

@mtias yes, updated in #4157

youknowriad added General Interface Parts of the UI which don't fall neatly under other labels. Needs Design Feedback Needs general design feedback. labels Dec 21, 2017

youknowriad self-assigned this Dec 21, 2017

youknowriad requested a review from jasmussen December 21, 2017 10:57

youknowriad force-pushed the update/publish-dropdown-2-panel branch from 77c2ffe to 82963bd Compare December 21, 2017 10:59

youknowriad requested a review from karmatosed December 21, 2017 10:59

gziolo mentioned this pull request Dec 21, 2017

Editor: Publish button rendered partially off the screen #3939

Closed

youknowriad force-pushed the update/publish-dropdown-2-panel branch from 0a8d83a to 240178f Compare December 21, 2017 12:51

youknowriad added 4 commits December 21, 2017 15:31

Chrome: Replace the publish dropdown by a sidebar panel

6c8ddf2

Chrome: Allow multiple sidebars

131ab94

Chrome: Slide in animation for the publish sidebar

100e210

Chrome: fix hasOpenSidebar selector

ae83845

youknowriad force-pushed the update/publish-dropdown-2-panel branch from a274aeb to ae83845 Compare December 21, 2017 14:34

Chrome: Fix animation speed of the publish sidebar

a8c3906

youknowriad requested review from jorgefilipecosta and mcsf December 21, 2017 16:40

atimmer reviewed Dec 22, 2017

View reviewed changes

atimmer approved these changes Dec 22, 2017

View reviewed changes

atimmer mentioned this pull request Dec 22, 2017

Add an API to add a plugin sidebar #4109

Closed

3 tasks

youknowriad merged commit 21e5b26 into master Dec 22, 2017

youknowriad deleted the update/publish-dropdown-2-panel branch December 22, 2017 15:13

mcsf reviewed Dec 22, 2017

View reviewed changes

afercia mentioned this pull request Dec 27, 2017

Publishing Flow accessibility #4187

Closed

noisysocks mentioned this pull request Jan 4, 2018

Chrome: Publish sidebar toggle state persists between page loads #4248

Closed

Conversation

youknowriad commented Dec 21, 2017

Uh oh!

jasmussen commented Dec 21, 2017

Uh oh!

youknowriad commented Dec 21, 2017

Uh oh!

jasmussen commented Dec 21, 2017

Uh oh!

jasmussen commented Dec 21, 2017

Uh oh!

youknowriad commented Dec 21, 2017

Uh oh!

jasmussen commented Dec 21, 2017

Uh oh!

atimmer left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mcsf left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mtias commented Dec 26, 2017

Uh oh!

youknowriad commented Dec 26, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants