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

Chrome: Replace the publish dropdown by a sidebar panel #4119

Merged
merged 5 commits into from Dec 22, 2017

Conversation

@youknowriad
Contributor

youknowriad commented Dec 21, 2017

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.

screen shot 2017-12-21 at 11 56 23

Next PRs will focus on:

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

This comment has been minimized.

Show comment
Hide comment
@jasmussen

jasmussen Dec 21, 2017

Contributor

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:

screen shot 2017-12-21 at 12 22 35

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!

Contributor

jasmussen commented Dec 21, 2017

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:

screen shot 2017-12-21 at 12 22 35

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

This comment has been minimized.

Show comment
Hide comment
@youknowriad

youknowriad Dec 21, 2017

Contributor

@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

Contributor

youknowriad commented Dec 21, 2017

@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

This comment has been minimized.

Show comment
Hide comment
@jasmussen

jasmussen Dec 21, 2017

Contributor

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:

  1. Make this action only for the "Publish", and not for "Update" or "Schedule"
  2. Build an opt-out button in the bottom of the publish box, and in the ellipsis menu.
  3. 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.

Contributor

jasmussen commented Dec 21, 2017

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:

  1. Make this action only for the "Publish", and not for "Update" or "Schedule"
  2. Build an opt-out button in the bottom of the publish box, and in the ellipsis menu.
  3. 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

This comment has been minimized.

Show comment
Hide comment
@jasmussen

jasmussen Dec 21, 2017

Contributor

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

Contributor

jasmussen commented Dec 21, 2017

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

@youknowriad

This comment has been minimized.

Show comment
Hide comment
@youknowriad

youknowriad Dec 21, 2017

Contributor

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

Contributor

youknowriad commented Dec 21, 2017

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

This comment has been minimized.

Show comment
Hide comment
@jasmussen

jasmussen Dec 21, 2017

Contributor

Ah cool. Yeah let's try .1s

Contributor

jasmussen commented Dec 21, 2017

Ah cool. Yeah let's try .1s

@youknowriad youknowriad requested review from mcsf and jorgefilipecosta Dec 21, 2017

@atimmer

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

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

This comment has been minimized.

@atimmer

atimmer Dec 22, 2017

Member

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

@atimmer

atimmer Dec 22, 2017

Member

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

This comment has been minimized.

@youknowriad

youknowriad Dec 22, 2017

Contributor

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.

@youknowriad

youknowriad Dec 22, 2017

Contributor

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.

This comment has been minimized.

@youknowriad
@youknowriad

This comment has been minimized.

@jasmussen

jasmussen Dec 22, 2017

Contributor

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.

@jasmussen

jasmussen Dec 22, 2017

Contributor

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.

This comment has been minimized.

@youknowriad

youknowriad Dec 22, 2017

Contributor

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

@youknowriad

youknowriad Dec 22, 2017

Contributor

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

@atimmer atimmer referenced this pull request Dec 22, 2017

Closed

Add an API to add a plugin sidebar #4109

3 of 3 tasks complete

@youknowriad youknowriad merged commit 21e5b26 into master Dec 22, 2017

3 checks passed

codecov/project 39.16% (+0.07%) compared to 9c6143b
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@youknowriad youknowriad deleted the update/publish-dropdown-2-panel branch Dec 22, 2017

@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:

gutenberg-publish-dropdown-to-panel

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:

customizer-widgets-spin-icon

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

This comment has been minimized.

@mcsf

mcsf Dec 22, 2017

Contributor

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

@mcsf

mcsf Dec 22, 2017

Contributor

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

This comment has been minimized.

@youknowriad

youknowriad Dec 22, 2017

Contributor

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

@youknowriad

youknowriad Dec 22, 2017

Contributor

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

This comment has been minimized.

@mcsf

mcsf Dec 22, 2017

Contributor

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

mcsf Dec 22, 2017

Contributor

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

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

This comment has been minimized.

@mcsf

mcsf Dec 22, 2017

Contributor

Minor, but we could extend mapDispatchToProps:

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

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

@mcsf

mcsf Dec 22, 2017

Contributor

Minor, but we could extend mapDispatchToProps:

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

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

'has-fixed-toolbar': hasFixedToolbar,
} );
const closePublishPanel = () => onToggleSidebar( 'publish', false );

This comment has been minimized.

@mcsf

mcsf Dec 22, 2017

Contributor

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

mcsf Dec 22, 2017

Contributor

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

*/
export function toggleSidebar( isMobile ) {
export function toggleSidebar( sidebar, force ) {

This comment has been minimized.

@mcsf

mcsf Dec 22, 2017

Contributor

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.)

@mcsf

mcsf Dec 22, 2017

Contributor

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.)

This comment has been minimized.

@youknowriad

youknowriad Dec 25, 2017

Contributor

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

@youknowriad

youknowriad Dec 25, 2017

Contributor

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

@mtias

This comment has been minimized.

Show comment
Hide comment
@mtias

mtias Dec 26, 2017

Contributor

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

Contributor

mtias commented Dec 26, 2017

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

@youknowriad

This comment has been minimized.

Show comment
Hide comment
@youknowriad

youknowriad Dec 26, 2017

Contributor

@mtias yes, updated in #4157

Contributor

youknowriad commented Dec 26, 2017

@mtias yes, updated in #4157

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment