-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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: Add a publish dropdown v1 #2887
Conversation
Codecov Report
@@ Coverage Diff @@
## master #2887 +/- ##
==========================================
+ Coverage 34.07% 35.54% +1.46%
==========================================
Files 192 197 +5
Lines 5675 6201 +526
Branches 996 1125 +129
==========================================
+ Hits 1934 2204 +270
- Misses 3165 3366 +201
- Partials 576 631 +55
Continue to review full report at Codecov.
|
YOU ARE A BULLET TRAIN! CHOO CHOOO! I really dig this. Thanks so much for working on it. This particular implementation seems to be using a "split button" approach, where if you click the left side of the button, you publish, if you click the right side, you get the dropdown. Correct? The alternative approach would be a single button that always invoked the dropdown, no matter where you clicked. Both approaches have their pros and cons. The split button gives the benefit of quicker publish access, though perhaps at the cost of simplicity. More of a power user feature, so to speak. This can be alleviated by design: The split button design is perhaps strongest when the post is already published, and needs to be updated, where it might make sense to skip the dropdown. Personally I'm partial to the single button approach. It makes it extremely easy to understand what happens — the first time you click the button, you learn what's up, and from then on you always know it. This also puts the actual publish action behind a "review and confirm", which as I've suggested in the past is desirable friction, so you don't accidentally publish. I'm super okay with trying either, though I'd love some quick thoughts by @karmatosed, @mtias or @folletto. When we decide whether to go split button or single button, some tiny tweaks should be made.
⭐️ ⭐️ ⭐️ ⭐️ ⭐️ work, Riad, you get a medal! 🥇 |
Quick thought: I'd prefer to have an intermediate dropdown/sidebar all the times. Rationale:
|
3565f8c
to
64fc77c
Compare
@youknowriad this is awesome! I worry a little that the 'just publish' and potential for slipping is a bit increased here. I would say it should only happen in the drop down. |
@folletto can you clarify the above a bit? Do you mean like how it works on WordPress.com? |
64fc77c
to
64572dd
Compare
64572dd
to
3fc640b
Compare
Probably need some polish but it's only opening the dropdown right now. |
Yes, I mean confirming your position here:
And also clumsly hinting — I didn't want to deviate this thread — that maybe a sidebar would be better than a dropdown for that because:
So I think either way it would work — but if possible I'd explore using a sidebar instead of a dropdown. |
Solid points, Davide. The "Publish..." phrase on wp.com seems nice, it keeps it a single button without the chevron.The room that the sidebar brings is also welcome, and if we could make it pluggable would be really nice. A sidebar also solves the issue of dropdowns inside dropdowns (i.e. date picker etc.) The primary downside of the sidebar as I see it (but this could very well just be me) is that it causes a jarring jog if it invokes when you have otherwise dismissed the Settings sidebar. Does it make sense for this to be a different sidebar? One that "covers" the content, exists on a higher plane than the editor itself and slides in with a shadow under it? Something like the following, but polished perhaps? There's some wonkiness with that pattern to work out, though — it's hard to find a good spot for that publish button. LMK if you want the sketch file. @youknowriad If we were to go with a sidebar instead of a dropdown, as Davide has suggested, would it be a ton of work to change this branch? What would make it easier/harder? What are your thoughts? |
This is a good point, I'm not sure what the answer is. The mockup might be a good way to tackle it: still covers the content, but still improves on all the other points, and avoids the jog. Maybe when it covers the "top" area, which matches the white toolbar at the top with "Publish..." button, just has a title, so it's "inactive", and the actual publish button is somewhere below? |
Yeah, seems like if we did want to go this direction it's something we need to solve. I like the idea that the publish button is right under, so if you know what you're doing you can "double click". But on the other hand it might not also be too easy to confirm then. It's hard finding the right amount of friction here. |
Technically, it can be challenging. If we go this road, I think we should do it in a separate PR, because it's more likely to impact the current settings sidebar as well. I personally have a preference for this:
|
Keeping it around is one of the reason of the current design: we have two kinds of users, one that will keep it open, and one that will keep it closed. Both exist, but they have different patterns. It's very important for the inspector sidebar to look natural both open and closed. :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Travis complains that tests for editor/header/publish-button/index.js
are failing.
Code wise I like this proposal a lot. It's almost ready to land if we would omit all UX considerations that are discussed above :)
} from '../../selectors'; | ||
|
||
function PublishWithDropdown( { isSaving, isPublishable, isSaveable } ) { | ||
const isButtonEnabled = ! isSaving && isPublishable && isSaveable; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is possible to expose it from connect
as one prop.
@@ -0,0 +1,8 @@ | |||
.editor-publish-with-dropdown { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It nicely replaces the existing override 👍
return ( | ||
<Dropdown | ||
position="bottom left" | ||
className="editor-publish-with-dropdown" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Newbie question, file name is editor/header/publish-with-dropdown/index.js
. A class name is combined from editor
keyword and folder name
. Is that a pattern we use and the reason that header
subfolder is ignored?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that's how we do it right now :)
isBeingScheduled, | ||
user, | ||
} ) { | ||
const isContributor = user.data && ! user.data.capabilities.publish_posts; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very nice idea to extract this logic in a variable 👍
Is there any plan to have a similar way to access API data as we have with Redux selectors? I can imagine we might want to use isContributor
check in other places. At the moment we would have to duplicates what we have here. What I like the most about selectors is that they hide the structure of data representation. /cc @aduth
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably related to how with make withApiData
state aware
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Selectors are just functions which happen to follow a particular first-argument convention. If it became a common pattern, I could see how it might be useful to create better organization around operating on API data. However, the implementation of withAPIData
makes some compromises hinging on an assumption that its usage is very ad-hoc. To @youknowriad's point, if we were to make this more organized, we might similarly want to rethink how it works in the first place. One example might be to support a separate argument on the higher-order component akin to react-redux
's mapStateToProps
which helps provide a barrier between the component implementation and the underlying data structure.
<Dashicon icon="arrow-down" /> | ||
</Button> | ||
) } | ||
renderContent={ ( { onClose } ) => <PublishDropdown onSubmit={ onClose } /> } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice way to close the dropdown on submit.
} ) { | ||
const isButtonEnabled = user.data && ! isSaving && isPublishable && isSaveable; | ||
const isContributor = user.data && ! user.data.capabilities.publish_posts; | ||
|
||
let buttonText; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
An excellent idea to put this logic into its own component.
Thanks for the thoughts, Riad.
Is there a third option, more like what's mocked up here, where the publish sidebar is something entirely different than the inspector sidebar — an overlay that floats above the main canvas. If the inspector sidebar is open, it will be covered by the publish sidebar. If the inspector sidebar is not open, the content below will be overlaid. You could almost think of this as a different design for the dropdown. Or am I missing something? I'm personally still a fan of the dropdown, but I hear Davide and I'm thinking of solutions that might meet in the middle. |
My concerns with this is: "Another UI pattern to grasp for the user", that's why I thought about reusing this pattern for all sidebars. We already have dropdowns, and we already have sidebars but happy to go with whichever you'll think is the best approach. |
Good point. I'll think a bit about this. |
We have a dropdown on the left side and quite random interactions on the right side of the top bar, but when you see the arrow down icon then you know what to expect. In the end it all depends on how much content we want to render in this new area. It makes a lot of sense to leave it as it is with the current shape. I'm afraid using sidebar would end up having lots of whitespaces if we don't add more options there. |
Since the dropdown is implemented, it might be worth going with that for now, and getting feedback. In the mean time I will explore some alternate routes as well, see what patterns we can take inspiration from.
I would suggest one thing that's more important than this (and is likely best discussed separately) — it is more important to make this area pluggable. |
+1,000 |
Here's what LivingDocs does: That is — the entire content area below changes. This may be a bit drastic than changing just the sidebar. The other common pattern I've seen, Medium, notion.so, Spark, they all show a dropdown of sorts. This dropdown can have various sizes and be dismissable or not with a click outside it, but it's definitely a pattern. I don't know which approach is best — I still lean towards a dropdown, even if it's a big one, as I feel like it is the least disruptive to the rest of the UI, scales better to mobile, and is also fairly lightweight when it comes to our adding intentional friction to the publishing experience. That is — I'm not at all against us trying other things, but would it be sensible to polish this PR up, merge as is, and then iterate? |
On mobile I'd just cover the entire screen, as there's little value imho in having something that uses the whole screen of space but keeps some borders just to hint "it's a dropdown". Full screen would also make scroll issues less problematic. The comment above is not to say "do a sidebar", just to point out that regardless on mobile it's probably more sensible that regardless of the desktop approach it should go full screen.
I think so. Regardless of the final choice of dropdow/sidebar — the behaviour we want is still the same in terms of interaction. |
That sounds good to me. 👍 👍 for merge from me, then we can get a feel for it! |
Ok let's ship this |
Is there any documentation for plugins extending this? |
@aaronjorbin This is not yet extendable, It should probably be discussed in #1352 |
This PR tries to implement the v1 of the publish dropdown closes #708
It is intended as a PR to try the technical viability and discuss this, rather than the final version of this dropdown.
Screenshots