Problem/Motivation

In a recent study of the content authoring experience done by Dharmesh users found the newly introduced "Save and keep published" overly long and confusing.

Overly complex solution.

Proposed resolution

Because "Save and keep published"/"Save and keep unpublished" are both long and awkwardly worded, it makes sense to change them to a boolean_checkbox simplified to Published underneath which "Save", "Preview", and "Cancel" buttons would be included.

Also revert #2423153: Add menu from the editing page doesn't save the changes issue, since now we will have only one "Save" button (with the previously mentioned Published status checkbox) and we don't want to have these 4 buttons anymore:

  • Save and unpublish
  • Save and keep unpublished
  • Save and publish
  • Save and keep published

This is how it should look like now:

Proposed solution.

Remaining tasks

  • Create patch
  • Reviews

User interface changes

  • Change interface for publishing and unpublishing content.
Files: 
CommentFileSizeAuthor
#145 interdiff-142-145.txt575 bytesJo Fitzgerald
#145 2068063-145.patch57.09 KBJo Fitzgerald
#143 Selection_116.png14.62 KBBerdir
#143 Selection_115.png19.86 KBBerdir
#142 2068063-142-interdiff.txt1.45 KBBerdir
#142 2068063-142.patch57.1 KBBerdir
#140 2068063-140.patch56.57 KBBerdir
#135 2068063-135.patch56.5 KBtimmillwood
#135 interdiff.txt2 KBtimmillwood
#132 2068063-132.patch55.16 KBtimmillwood
#132 interdiff.txt1.16 KBtimmillwood
#130 change_save_and_keep-2068063-130-interdiff.txt7.4 KBBerdir
#130 change_save_and_keep-2068063-130.patch56.03 KBBerdir
#128 change_save_and_keep-2068063-128.patch50.09 KBchr.fritsch
#124 change_save_and_keep-2068063-124-8.2.patch50.45 KBBerdir
#123 change_save_and_keep-2068063-123.patch50.24 KBBerdir
#119 change_save_and_keep-2068063-119.patch50.36 KByongt9412
#117 change_save_and_keep-2068063-117.patch50.33 KByongt9412
#115 Screenshot from 2016-10-11 10-34-52.png26.73 KByongt9412
#115 change_save_and_keep-2068063-115.patch46.9 KByongt9412
#94 save-published.png18 KByoroy
#82 node-save-published-2068063-82.patch47.16 KBtduong
#82 interdiff-2068063-80-82.txt226 bytestduong
#58 node-save-published-2068063-58.patch4.24 KBBerdir
#58 node_save_published.png17.27 KBBerdir
#45 new_save_pattern.png167.35 KBtkoleary
#44 new_save_pattern.png167.95 KBtkoleary
#35 node_edit_draft.png39.12 KBesod
#35 node_edit.png37.06 KBesod
#35 node_add.png41.42 KBesod
#24 change_save_and_keep-2068063-24.patch20.44 KBnlisgo
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 79,492 pass(es). View
#24 interdiff-2068063-19-24.txt805 bytesnlisgo
#11 change-save-and-keep-published-to-update-2068063-11.patch20.37 KBesod
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 79,406 pass(es), 83 fail(s), and 9 exception(s). View
#11 before.node_add.png42.91 KBesod
#11 before.node_edit.png46.46 KBesod
#11 after.node_add.png41.66 KBesod
#11 after.node_edit.png40.98 KBesod
#11 before.node_edit_draft.png42.13 KBesod
#11 after.node_edit_draft.png39.4 KBesod
#13 change-save-and-keep-published-to-update-2068063-13.patch2.55 KBesod
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 78,574 pass(es), 164 fail(s), and 20 exception(s). View
#19 interdiff-2068063-11-19.txt3.14 KBnlisgo
#19 change_save_and_keep-2068063-19.patch20.65 KBnlisgo
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 79,569 pass(es), 3 fail(s), and 0 exception(s). View
#26 interdiff-2068063-24-26.txt6.1 KBnlisgo
#26 change_save_and_keep-2068063-26.patch20.48 KBnlisgo
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch change_save_and_keep-2068063-26.patch. Unable to apply patch. See the log in the details link for more information. View

Comments

dawehner’s picture

Does that mean that we have either:

  • Update
  • Save and unpublish

and in the case of unpublished

  • Update
  • Save and publish

There seems to be no context anymore that a post is published or unpublished.

tkoleary’s picture

That's a good point. We need to deal with drafts as well. The common practice is this:

  • Update
  • Save as draft

and in the case of unpublished

  • Update draft
  • Save and publish

We have shied away from using "draft" I think because of worries about confusion with workflow, however I think that's the tail wagging the dog. Draft is the common nomenclature for a post that is not published.

The other issue with this is that we are overloading the submit with when we have "save as unpublished" which both saves and unpublishes the node. Unpublish is available in the content list which I suspect is where users commonly use it. Removing unpublish from the submit makes "save as draft" what most users expect "Save a revision that I can work on without unpublishing the current published version". This is possible in core now and I'm not sure why we're not using it.

tkoleary’s picture

Issue summary: View changes

Just noticed that we now have "save and keep unpublished". Oh my god.

How many verbal knots are we going to tie ourselves in before we realize that this is wrong?

tkoleary’s picture

Version: 8.0.x-dev » 8.0.0-beta1
Bojhan’s picture

Version: 8.0.0-beta1 » 8.0.x-dev
Issue tags: -#submit, -add new content, -Add Save +Usability

We fix this in dev. Its indeed a bit weird, what we are doing with that. A patch for this should be easy.

tkoleary’s picture

@bojhan

What's your preferred nomenclature?

Bojhan’s picture

@tkoleary I would favour the terminology that you use.

tkoleary’s picture

@Bojhan

Cool, so just to update since #2 did not encompass the whole interaction it would be:

New node:

Save and publish | Save draft

On saving draft the DSM could say "Draft has been saved. [Content title] is no longer published. Click "save and publish" to make this draft live.

Draft (unpublished)

Update draft | Save and publish

On updating draft the DSM could say "Draft has been updated. Click "save and publish" to make this draft live.

Published node

Update | Save draft

On updating draft the DSM could say "[content title] has been updated. Click "save draft" to unpublish while editing"

The only changes here from #2 are that this covers the new node, which does not follow "update" since it has never been saved, and that I changed "save as draft" to "Save draft" since commonly "save as" operations imply a copy has been created which is not the case here.

Bojhan’s picture

Status: Active » Needs work
Issue tags: +Novice

Good point about the "save as". It probably makes sense to split of the DSM changes, although connected I imagine it needs a bit more thinking than the labels because we don't do that now.

Patches please!

tkoleary’s picture

@dawehner

Since Bojhan and I are both non-coding designers would you mind rolling a patch of this? :)

esod’s picture

Status: Needs work » Needs review
FileSize
20.37 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 79,406 pass(es), 83 fail(s), and 9 exception(s). View
42.91 KB
46.46 KB
41.66 KB
40.98 KB
42.13 KB
39.4 KB

These show up in quite a few tests. Let's see what happens during testing.

The DSMs for Draft messages are probably be a bit more involving. The isNew() function helps to create a distinction between the "... has been created." message and "... has been updated." message. I don't see an isDraft() function which would make it easy to set conditions for the "... has been updated." message. I'll keep looking.

BEFORE node_add.png







AFTER node_add.png







BEFORE node_edit.png







AFTER node_edit.png







BEFORE node_edit_draft.png







AFTER node_edit_draft.png


Status: Needs review » Needs work

The last submitted patch, 11: change-save-and-keep-published-to-update-2068063-11.patch, failed testing.

esod’s picture

Status: Needs work » Needs review
FileSize
2.55 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 78,574 pass(es), 164 fail(s), and 20 exception(s). View

Well, maybe I'm not supposed to change the test scripts. Let's try the patch with just the changes on NodeForm.php.

Status: Needs review » Needs work

The last submitted patch, 13: change-save-and-keep-published-to-update-2068063-13.patch, failed testing.

Bojhan’s picture

I am just wondering - doesn't "save as draft" actually more accurately describe it. Because you are actually having two different "versions" of the same thing here. The "live" one and the "future revision" one. It is special to the other ones, were you just have the one version.

@tkoleary what do you think?

yoroy’s picture

Nice labelling improvements me thinks. I do wonder about this one though (I think this is what bojhan means as well):

AFTER node_edit.png

Wouldn't it have to be Save *as* in this case because we're indeed splitting off a draft version from an existing, published node?

esod’s picture

Sure. We can change that one or any other other one that @tkoleary likes.

I'd like to know why my patch fails testing. It works on my local and it works on SimplyTest.me.

nlisgo’s picture

@esod, let me take a look.

nlisgo’s picture

FileSize
20.65 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 79,569 pass(es), 3 fail(s), and 0 exception(s). View
3.14 KB

Well I'm of no use. I seem to be getting the error when testing FileFieldDisplayTest locally on 8.0.x HEAD locally!!

Is it possible that a bug has crept into core and this in unrelated?

I am going to upload a new patch because I think you mixed up some of the changes in text.

See attached interdiff.

nlisgo’s picture

Status: Needs work » Needs review

The last submitted patch, 19: change_save_and_keep-2068063-19.patch, failed testing.

yoroy’s picture

Getting closer though, only 3 test fails left!

esod’s picture

Thanks @nlisgo for your help. Will you describe in detail what you are doing when you say "I seem to be getting the error when testing FileFieldDisplayTest locally on 8.0.x HEAD locally!!"

I'm following the instructions from https://www.drupal.org/node/2116263, but I'd like to know more about the tests so I can get more patches to pass.

nlisgo’s picture

FileSize
20.44 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 79,492 pass(es). View
805 bytes

Here's another attempt

Bingo!

nlisgo’s picture

@esod, I'll look into the issues I was having with the tests and perhaps contact you directly if I have a breakthrough. But my problems are definitely unrelated to this one.

nlisgo’s picture

FileSize
20.48 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch change_save_and_keep-2068063-26.patch. Unable to apply patch. See the log in the details link for more information. View
6.1 KB

Incorporated the feedback from #15 and #16 changing 'Save draft' to 'Save as draft'.

esod’s picture

Sweet. I'll have a look at your passed patch and the interdiff. I'm sure that will help me too.

esod’s picture

Now that we've resolved the issue with the text in the button(s), how do you think we could customize the draft messages per @tkoleary's suggestions in #8? There's only $insert and isNew() to work with at the moment.

Berdir’s picture

Wouldn't it have to be Save *as* in this case because we're indeed splitting off a draft version from an existing, published node?

No we don't. We unpublish the node and save that.

Which I think means this pretty misleading, Save as Draft or Save Draft not making a big difference. "Save and unpublish" is exactly what it does..

Bojhan’s picture

@Berdir That is a huge WTF, why the hell do we do that?

esod’s picture

It's always worked this way. In D7 when you un-publish a node and Create a new revision, the node's status column in the database is changed to 0 and the vid column (revision column) is given to a new integer.

In D8, the data structure has changed somewhat so that when you un-publish a node and Create a new revision, the node's vid column is given a new integer and the node_field_revision's table status column is changed to 0.

Either way the action is the same. A revision doesn't make a new node, it changes things for the existing node.

So, Save and unpublish is the literal action, but it is suggested that Save and unpublish (and its variants) will be confusing to the end user. If the end user knows or will intuit that Save Draft/Update Draft will apply to unpublished nodes, the changes we want to implement will be fine.

Berdir’s picture

Yes, I'm not saying it is great the way it is, and there have been issues for a long time that wanted to change it, but that is how it works now.

So, Save and unpublish is the literal action, but it is suggested that Save and unpublish (and its variants) will be confusing to the end user. If the end user knows or will intuit that Save Draft/Update Draft will apply to unpublished nodes, the changes we want to implement will be fine.

This is an interesting paragraph :)

1. The issue summary only mentions the "Save and keep published" button as confusing, only later on the other labels have been discussed as well. So there doesn't seem to be any data to back those?
2. "If the end user knows X and Y, then Z will be clear". Isn't UX exactly about avoiding that you have to know things to be able to understand the UI?

I did a quick search, and there doesn't seem to be any relevent usage of "draft" in core, so this is introducing new words that aren't used in help/documentation yet.

Not sure, but maybe this issue should be limited to the "Save and keep published", where I think "Update" is a valid replacement.

There are also technical underlying problems, that are not changed by this issue but is still relevant. Because there are various different buttons based on the current state of a node, it is very hard for a contrib module to alter them. There is an old issue open about this.

zaporylie’s picture

Status: Needs review » Reviewed & tested by the community

I've successfully applied patch #26 and really like how it keeps submit button simple and clear. I want to have it in 8.0.x, so RTBC+1

Berdir’s picture

Status: Reviewed & tested by the community » Needs review

Based on the recent dicussions, I don't think this is RTBC?

esod’s picture

FileSize
41.42 KB
37.06 KB
39.12 KB

Although there is no use of the word draft in core up until now, I vote to start using it at this time, for this case. Every other combination I try ends up leading me to difficult phrases like Save and keep published/Save and keep unpublished. Those phrases are too wordy and I expect users will be frustrated by those phrases' seeming lack of clarity.

Of note is something in comment #2

We have shied away from using "draft" I think because of worries about confusion with workflow, however I think that's the tail wagging the dog. Draft is the common nomenclature for a post that is not published.

If "Draft is the common nomenclature for a post that is not published", I think we're good with the current changes. All software, even great software, relies on the user to "know" something about the software in order to use it. So is it unrealistic to expect the user to know drafts are unpublished or is it not? I say it is not.

Anyway, here are current screenshots of the proposed changes for the sake of the discussion.

node_add







node_edit







node-edit_draft


nlisgo’s picture

@esod the latest patch should have changed 'Save draft' to 'Save as 'draft'

tkoleary’s picture

@berdir @esod

It is not reasonable for the entire burden of comprehension to be placed on the button text alone which is why I introduced reference to the DSM back in #8.

That DSM language in that coupled with the new "draft" nomenclature resolves the issue and this should be the experience of the user:

  1. "I want to save this article but I don't want to publish it yet. Let's see what options I have"
  2. clicks drop button
  3. "Oh, here's a "save draft" option, that's what I want. Drafts are not live."
  4. clicks save draft
  5. DSM:
    Draft has been saved. [Content title] is no longer published. Click "save and publish" to make this draft live. Use "make revision" if you want to keep the node published while working on a new version.
  6. "Ah, when I make a draft it's no longer live, cool. I'll finish making my changes then publish it again"

    or

  7. "Oh, when I make a draft it's no longer live. I want it to stay live while I work, I'll make a revision instead."

We should *not* use "save as draft" because, as Berdir noted, we are saving the changes to the node, not to a new version of the node and "save as" is universally understood as "keep the version from where I began editing but save the changes I just made to a new version" which is not what we do.

To the comment that "draft" is not a word we currently use, we should employ off-the-island thinking here. "Draft" is a ubiquitous and well defined term fully loaded with borrowed connotations we can rely on to guide users unfamiliar with Drupal. Once it is in core it also nudges contrib workflow solutions towards using "revision" in their nomenclature, which is what we want.

Berdir’s picture

I'm not against using "draft", that's fine with me. And it seems great when creating new content ("Save and publish" vs. "Save draft").

Use "make revision" if you want to keep the node published while working on a new version.

Unfortunately, not even that is true. [x] Create new revision is essentially "Save as", it keeps the old values around as an old revision, but the active revision of the node will be unpublished and the node will no longer be accessible for normal users.

The API supports this concept with isDefaultRevision() ( new in 8.x, this will save but not make the new values the active revision), but we don't use that in the UI/Forms, because starting so is going to open a whole bunch of new issues. For example, I really don't know what will happen if you then edit the node again. It will probably edit the active revision again and you will have no way to edit your latest revision... and so on ;)

We have "Save and publish", so why not use "... and unpublish"?

Would then look like this:
New: Save and publish (default), Save draft
Update published: Update (default), Update and unpublish
Update unpublished: Update draft (default), Update and publish

Then everything that *changes* the publication state is very clear and short enough IMHO and we use can draft to shorten some of the currently too long labels.

esod’s picture

We definitely need to change the DSM for updates. It is difficult to change the DSM for updates because currently the save() method in class NodeForm has the ability to print only one possible message for an updated node. See the relevant line of code in core/modules/node/src/NodeForm.php, line 421.

Use "make revision" if you want to keep the node published while working on a new version.

Making a "new version" is not the same as "Create new revision". New revisions don't change a node's publish status. I can see publishers wanting to work on a new version while keeping the old version published. If I'm not mistaken, that's what new nodes are for.

tkoleary’s picture

@Berdir

That still looks a bit complex to me and I'm beginning to question the value of "update" if we use "draft". The reason for suggesting update was to remove the clumsy "save and keep published" but I think save is sufficient in both cases if the other option is "save draft".

There is no difference when you save and publish a new node or when you save/update/save and keep published a published node. Presumably the language was introduced to remind the user that the node is live and will be immediately public on save, but that information is also already printed elsewhere in the UI and there is no evidence to show that users would not just know this.

Given the subtleties of this, I made paper prototypes of three different versions of this flow and hallway tested them on five users with knowledge of Drupal ranging from virtually none to advanced.

  • Even when the word "draft" was not in the prototype, when asked what would happen if the user clicked "save and unpublish" or "save" when the other option was "save and publish", three users said "it would be a draft" unprompted.
  • On flows where is said "update" instead of "save" or "save and publish" three users were confused as to what was being updated and whether or not it would be live.
  • On flows where the default changed from "save and publish" to "update" or "Save" users found the change jarring, eg. "why did it change from save and publish"?
  • Two users asked why save was needed with publish, they assumed publishing would save
  • The most experienced Drupal user thought that "Save and unpublish" saved a working draft but the original node was still live but he was the outlier
  • My takeaways from this were:
  • The word "draft" is very useful
  • The default action should not change
  • The word "unpublish" is essential for users to understand that the action takes down the live version

When changed to the following and tested on a new set of users the following language produced less confusion:

New node:
Publish
Save draft

Published node:
Publish
Save & unpublish

Draft (unpublished) node
Publish
Save draft

BUT, all users still found "save and unpublish" strange and asked why "unpublish" could not be a separate action from save. I am inclined to agree with that and I wonder if we should consider going to a button and toggle model eg.

New node:

[toggle] Published |unpublished
[button] Save

Published node:

[toggle] Published | unpublished
[button] Save

Unpublished node:

[toggle] Published | unpublished
[button] Save

Seems simpler

Berdir’s picture

Toggle/checkbox seems simple to me too, and it would solve #1901216: Modules cannot reliably enhance or alter the node form anymore along the way (that is the issue I mentioned above), as it would mean that we only have a single button that a module needs to alter.

Berdir’s picture

It is kind of funny, as it would essentially be the same as 7.x again, the published checkbox would just be more visible.

tkoleary’s picture

@Berdir Yes, it's ironic.:)

I think the lesson is that consistency is better especially in instances like this where the pattern is perhaps the most commonly used interaction in Drupal and many many users have established expectations for how it will behave.

I think if we land here it is definitely an improvement over 7 and also puts us in a place that is simpler and less ambiguous than wordpress, joomla, Adobe or Sitecore, all of which have more complicated patterns.

tkoleary’s picture

FileSize
167.95 KB

It could look like this. I prefer state to action and radios to checks here because it works better in all states of the node.

save

Status could possibly also be "Change status"

I also added cancel because it's the more common pattern, though perhaps that warrants another issue.

tkoleary’s picture

FileSize
167.35 KB

The last submitted patch, 26: change_save_and_keep-2068063-26.patch, failed testing.

disasm’s picture

Lets get the issue summary updated with before/after screenshots of the proposed layout.

brandonpost’s picture

I'm at the sprint at DrupalCon LA, and I will start working on a patch to implement the changes proposed by @tkoleary in #44 & #45. In #44, he asks whether adding a 'Cancel' button should be part of the scope for this issue. Does anyone have thoughts about this?

Berdir’s picture

+1 to the changes suggested there.

I don't think that cancel belongs here, at least not in a first iteration.

Note: With that, we might actually be able to use a widget again for the status. To get started with that, add a similar setDisplayOptions() call to the status field as the promote/sticky fields already have. The difference is that you want to use options_buttons instead of boolean_checkbox, although we probably need to work a bit on the labels. To get those, you need to set the on_label and off_label settings.

Edit: In Node::baseFieldDefinitions()

I'm in IRC if you have questions.

janaabiakar’s picture

Hi @Brandonpost, I am at the DrupalCon LA also, I am a UX/UI designer. I think it's important to add a Cancel button as suggested by @tkoleary.
People sometimes make mistakes while inputting their content and it's helpful to just hit a button to disregard all the changes that have been applied to a form.

brandonpost’s picture

@Berdir, thanks a lot for the guidance! This is taking longer than I thought it would since I'm still getting to know Drupal 8. I have to leave now to catch a flight, but I will try to work on this more this weekend.

@janaabiakar, I agree there should be a Cancel button here for the reason you gave. My question was more whether or not adding a Cancel button falls within the scope of this issue, or if a separate issue should be opened to allow for more discussion about the Cancel button. One question I have is where to send the user if they click the Cancel button. I think we would want to send the user back to wherever they came from. I'm not sure the best way to get this information in D8, if we just use $_SERVER['HTTP_REFERER'], or if there is a better way.

adamexmachina’s picture

Status: Needs review » Needs work

This issue needs work and is not yet ready for review.

adamexmachina’s picture

Status: Needs work » Needs review
adamexmachina’s picture

Issue summary: View changes
Issue tags: -Novice, -Needs issue summary update

I am removing the Novice tag from this issue because the summary has been updated and there are no more novice tasks to be completed at this time.

brandonpost’s picture

Status: Needs review » Needs work

@adamexmachina, you were right that this issue still needs work and is not ready for review.

drifter’s picture

Once you have content translation enabled, the save button's label changes to "Save and keep published (this translation)", which is even more verbose and harder to parse.

Berdir’s picture

Version: 8.0.x-dev » 8.1.x-dev
Status: Needs work » Needs review
FileSize
17.27 KB
4.24 KB

Maybe we can get this into 8.1.0?

This is a patch that uses a checkbox. That allows us to use the standard checkbox widget and is trivial to implement, for radio with options, we'd need to use the boolean field type, which is not a trivial switch to make and definitely not just a UI thing.

The downside is that this will break dozens of tests as they all use the button label. But afaik we can break tests in minor releases, so that should be OK.

Status: Needs review » Needs work

The last submitted patch, 58: node-save-published-2068063-58.patch, failed testing.

tkoleary’s picture

@Berdir

Bravo

Berdir’s picture

+++ b/core/modules/node/src/Entity/Node.php
@@ -399,11 +399,18 @@ public static function baseFieldDefinitions(EntityTypeInterface $entity_type) {
+        'weight' => 15,

The default weight should probably be higher so that it is likely to be at the end, together with the Save button, e.g. 90 or so.

Also, it requires a cache clear to update the view displays. Maybe we want an explicit update function to add the component for all node view displays explicitly.

tduong’s picture

Status: Needs work » Needs review
FileSize
33.88 KB
41.06 KB

Fixed a bunch of tests. There are still tests failing and need some more work. Hints on how to proceed are welcome! ;)

There is a test (Drupal\menu_ui\Tests\MenuNodeTest) coming from #2423153: Add menu from the editing page doesn't save the changes. I think we can remove it or revert that commit, since we don't need these 4 buttons anymore.

And the another strange test (Drupal\node\Tests\ NodeFormButtonsTest), which use an assertButtons() method that check for the dropdown buttons, which we don't have anymore. It's pretty useless asserting the same button each time, so maybe we should change this assertButtons() method or adding a new one that checks for the visibility of our new 'Published' boolean_checkbox and if it is checked or something like that, what do you think?

Status: Needs review » Needs work

The last submitted patch, 62: node-save-published-2068063-62.patch, failed testing.

tduong’s picture

Status: Needs work » Needs review
FileSize
7.11 KB
44.31 KB

Rerolled. New:

Berdir’s picture

Status: Needs review » Needs work
+++ b/core/modules/file/src/Tests/FileFieldDisplayTest.php
@@ -73,8 +73,11 @@ function testNodeDisplay() {
+      'status[value]' => TRUE,

it said keep, so explicitly setting it should not be needed.

Same for a whole bunch of others too. if it said Save and keep *, just change to Save. only if it says Save and publish or Save and unpublish, then you need to set status.

The last submitted patch, 64: node-save-published-2068063-64.patch, failed testing.

tduong’s picture

Status: Needs work » Needs review
FileSize
15.96 KB
47.14 KB

Fixed the bugs @Berdir mentioned in #65 and the remaining failing tests.

Status: Needs review » Needs work

The last submitted patch, 67: node-save-published-2068063-67.patch, failed testing.

tduong’s picture

Status: Needs work » Needs review
FileSize
22.63 KB
45.05 KB

As discussed with @Berdir:

  • I've set 'status[value]' only when is Save and unpublish,
  • removed NodeFormButtonsTest
  • and added a test verifying if a node is unpublished after editing it.
Berdir’s picture

Status: Needs review » Needs work
  1. +++ b/core/modules/node/src/Tests/NodeEditFormTest.php
    @@ -159,7 +168,7 @@ public function testNodeEditAuthoredBy() {
     
         // Save the node without making any changes.
    -    $this->drupalPostForm('node/' . $node->id() . '/edit', [], t('Save and keep published'));
    +    $this->drupalPostForm('node/' . $node->id() . '/edit', NULL, t('Save'));
         $this->nodeStorage->resetCache(array($node->id()));
         $node = $this->nodeStorage->load($node->id());
         $this->assertIdentical($this->webUser->id(), $node->getOwner()->id());
    @@ -171,7 +180,7 @@ public function testNodeEditAuthoredBy() {
    
    @@ -171,7 +180,7 @@ public function testNodeEditAuthoredBy() {
     
         // Check that saving the node without making any changes keeps the proper
         // author ID.
    -    $this->drupalPostForm('node/' . $node->id() . '/edit', [], t('Save and keep published'));
    +    $this->drupalPostForm('node/' . $node->id() . '/edit', NULL, t('Save'));
         $this->nodeStorage->resetCache(array($node->id()));
         $node = $this->nodeStorage->load($node->id());
    

    The [] to NULL chnage here is unrelated.

  2. +++ b/core/modules/node/src/Tests/NodeRevisionsUiTest.php
    @@ -72,8 +72,7 @@ function testNodeFormSaveWithoutRevision() {
    -    $edit = array();
    -    $this->drupalPostForm('node/' . $node->id() . '/edit', $edit, t('Save and keep published'));
    +    $this->drupalPostForm('node/' . $node->id() . '/edit', NULL, t('Save'));
    

    here too.

  3. +++ b/core/modules/options/src/Tests/OptionsFieldUITest.php
    @@ -330,9 +330,9 @@ function testNodeDisplay() {
    -      $this->fieldName => '1',
    +      $this->fieldName => TRUE,
         );
    

    Also an unrelated change.

  4. +++ b/core/modules/system/src/Tests/Update/UpdatePathRC1TestBaseFilledTest.php
    @@ -122,7 +122,7 @@ public function testUpdatedSite() {
         $this->assertRaw('0.01');
    -    $this->drupalPostForm('node/8/edit', [], 'Save and keep published (this translation)');
    +    $this->drupalPostForm('node/8/edit', NULL, 'Save (this translation)');
         $this->assertResponse(200);
    
    +++ b/core/modules/system/src/Tests/Update/UpdatePathTestBaseFilledTest.php
    @@ -122,7 +122,7 @@ public function testUpdatedSite() {
         $this->assertRaw('0.01');
    -    $this->drupalPostForm('node/8/edit', [], 'Save and keep published (this translation)');
    +    $this->drupalPostForm('node/8/edit', NULL, 'Save (this translation)');
         $this->assertResponse(200);
    

    same.

  5. +++ b/core/profiles/standard/config/install/core.entity_form_display.node.article.default.yml
    @@ -58,6 +58,12 @@ content:
         third_party_settings: {  }
    +  status:
    +    type: boolean_checkbox
    +    settings:
    +      display_label: true
    +    weight: 100
    +    third_party_settings: { }
       sticky:
    

    I think we still need to write an update function for this.

    Add node_update_N (where N is bigger than the last).

    Load all form displays for nodes, add status with these settings, save.

tduong’s picture

Status: Needs work » Needs review
FileSize
3.7 KB
44.89 KB

Fixed unrelated changes. I'll added that function later.

The last submitted patch, 69: node-save-published-2068063-69.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 71: node-save-published-2068063-71.patch, failed testing.

tduong’s picture

Rerolled and added node_update_8004() in node.install

Status: Needs review » Needs work

The last submitted patch, 74: node-save-published-2068063-74.patch, failed testing.

tduong’s picture

Status: Needs work » Needs review
FileSize
1.75 KB
47.19 KB

Fixed a Save and keep published button committed from another issue, refactored node_update_8004().

Berdir’s picture

Status: Needs review » Needs work
  1. +++ b/core/modules/node/node.install
    @@ -225,13 +225,15 @@
       $query = \Drupal::entityQuery('entity_form_display')
    -    ->condition('targetEntityType', 'node');
    +    ->condition('targetEntityType', 'node')
    +    ->condition('mode', 'default');
    

    What else is there that's not default?

  2. +++ b/core/modules/node/node.install
    @@ -225,13 +225,15 @@
       foreach ($form_displays as $nid => $node) {
    -    // Assign status settings for the 'default' form mode.
    -    $form_displays
    +    $node
    

    Neither is correct. Use $form_display.

tduong’s picture

Status: Needs work » Needs review
FileSize
949 bytes
47.11 KB

Rerolled and done.

Status: Needs review » Needs work

The last submitted patch, 78: node-save-published-2068063-78.patch, failed testing.

tduong’s picture

Status: Needs work » Needs review
FileSize
486 bytes
47.16 KB

Misunderstood @Berdir's suggestion. Fixed.

Berdir’s picture

+++ b/core/modules/node/node.install
@@ -218,3 +219,25 @@ function node_update_8003() {
+  foreach ($form_displays as $id => $form_display) {
+    $form_display->setComponent('status', array(
+      'type' => 'boolean_checkbox',
+      'settings' => array(
+        'display_label' => TRUE,
+      ),
+    ))
+      ->save();

Indentation looks a bit off now. Patch looks good to me now otherwise.

Lets also update the issue summary, link to related issues that we're reverting to explain the big change in that test there.

tduong’s picture

tduong’s picture

Issue summary: View changes
xjm’s picture

@Berdir asked me to take a look at this issue in terms of whether it would be acceptable for a minor version.

Based on the nature of the change and the specific code changes (at least based on a cursory code review), this would be a good type of change to target for a minor version. It's disruptive to the node form structure (which is considered an internal API) and the user interface, which are both things we can change in minor versions per https://www.drupal.org/core/d8-allowed-changes#minor and https://www.drupal.org/core/d8-bc-policy.

The one concern I'd have is that in general we do not want to force people to adopt new UIs in minor versions -- we want to make it an opt-in for old installations and the default for new ones. "Fixing" the usability for existing sites is actually sometimes worse for UX because suddenly the existing users have to learn new patterns. (A memorable image in a DrupalCon Chicago keynote in 2011 was that it was akin to having someone come into your kitchen in the middle of the night and reorganize all your stuff for "better" kitchen usability, but that just means you suddenly no longer know where your spoons are. Before, you knew where they were, even if it was a dumb spot.) So is there any way we could sanely keep the old option without adding too much cruft? (If the answer is "no", that's not necessarily a reason not to do this, but we should at least discuss it.)

We'd want both usability and product manager signoff before committing the change. I also think it needs an upgrade path based on the config changes in there?

Thanks everyone! Usability improvements like this are great for the new minor version cycle.

xjm’s picture

Oh also, the current issue title disagrees with the issue summary and patch.

xjm’s picture

Adding a related issue from @berdir.

xjm’s picture

@swentel pointed out that node_update_8004() is in the patch, but it might still need an upgrade path test. Thanks @swentel!

tduong’s picture

Title: Change "Save and keep published" to "Update" » Change "Save and keep un-/published" buttons to a "Published" checkbox and an included "Save" button
Issue summary: View changes

Updated title and improved issue summary a little more .

Berdir’s picture

Keeping the existing behavior for existing sites would be possible but problematic.

Here's what we could do:

1. Right now, the update path ads the new status component as visible, like the default config. We could change that and hide it explicitly instead.
2. Either based on that or another, separate setting, we decide to show either the new single button or the old ones. Not sure if we need a separate setting, it doesn't really make sense to either show both (not sure what would actually win then, probably the buttons) nor neither.

However, I'm worried about the complexity that that would imply for core/contrib modules. My main reason for working on this is not the UX improvement. It's that this also results in a big DX improvement. We can for example close #1901216: Modules cannot reliably enhance or alter the node form anymore as a duplicate of this. We also no longer need any of the changes that #2423153: Add menu from the editing page doesn't save the changes introduced. A mistake that I'm sure lots of contrib and custom modules will make too.

If we keep the support for the old UI, we make that an even bigger problem and even more complicated to alter those buttons reliably for both existing and new sites. See for example https://www.drupal.org/project/override_node_options. This change *will* break that module (no matter which option we pick). But the nice thing is that the new code for it will be absolutely trivial. it just has to add two more lines to http://cgit.drupalcode.org/override_node_options/tree/override_node_opti... and can throw away all of http://cgit.drupalcode.org/override_node_options/tree/src/OverrideNodeOp.... (which is copied from core and adjusted for the new permission the module adds).

Berdir’s picture

Status: Needs review » Needs work

We can remove a bit more from NodeForm. The updateStatus() method and it's usage as an entity_builder directly above.

xjm’s picture

Issue tags: +Needs change record

Discussed with @Berdir. I think we should add a change record for this change given the impact on the user interaction and contrib.

Crell’s picture

Just a note, Workbench Moderation currently adds an arbitrary number of new alternatives to the drop-down button now. If that UI model changes, we still need to allow for an arbitrary number of additional "save and..." operations, because that's what WBM does. I'd also push back on leaving both old and new UIs intact, as that doubles the work for us to enhance that list. :-(

One of our designers suggested a select box with Save button rather than radios, as it would scale better to the potentially many transitions WBM could add.

Berdir’s picture

Select + save would I think be perfectly compatible with the proposed solution. You'd just hide the default status widget that is added for node forms, which you can easily do from the UI and instead show a widget for your own field.

yoroy’s picture

FileSize
18 KB

On the general notion of changing UI between minor versions: I don't think keeping the existing behaviour around for existing sites is a good idea. Apps update all the time on my phone, and I can still find the retweet and like buttons in the Twitter app too. It's not like we're rearranging the whole kitchen, more like swapping the knives and the spoons in the same drawer :-)

As for the proposed change, looking at the latest patch in #82 it looks like the new direction could work but the current design is not good enough yet:

The checkbox is not clearly related to the Save button, it might as well apply to the image field above. It’s a pity the radio button approach in #44 is so much more work, as it communicates a lot better what’s going on.

This is not a detailed design proposal but something like this to visually group the checkbox and the button needs to happen before we can commit this:

xjm’s picture

Thanks @yoroy, that all makes sense to me.

xjm’s picture

I wonder if we could do some sort of A/B testing for the proposed change once @yoroy's feedback is implemented, since we are reverting a previous fix that was also intended as a usability improvement?

xjm’s picture

So the previous issue also discussed several of these form element design choices.

Berdir’s picture

The problem is that Published is a widget. A user can see and move it around on the manage form display page. So we can't just add some simple CSS rules to change some spacing and add a visual separation. What if someone wants to show the checkbox below the title?

We have similar things with for example the node links, which are configurable too but by default, we show them separately in the node template and just care about shown or hidden from that UI. We could do something like that too, likely in seven.theme, where we already have a bunch of form alter for the node form. (Although I'm not a fan of that either, as it's close to impossible to customize that without a seven subtheme).

Bojhan’s picture

@xjm With our current practices/resources thats unlikely. I think whatever we decide we have to be very careful of its impact this is a 99% interface.

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.0-beta1 was released on March 2, 2016, which means new developments and disruptive changes should now be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Bojhan’s picture

Issue tags: -Needs usability review
webchick’s picture

phenaproxima’s picture

IMHO, the Published checkbox should be treated as a normal widget -- that is, allow the site builders to move it wherever they like. But it should be defaulted to appearing immediately before the form actions. If the user explicitly moves it from there, so be it.

And to prevent it from getting lost in the chaos of the full form, I think core should provide a bit of CSS to highlight the checkbox in some way, like a light grey background for the entire widget and its wrapper. Or, if we want to get really fancy, a background color that changes depending on the state of the checkbox (red if unpublished, green if published).

webchick’s picture

From talking to Bojhan it doesn't sound like this is ready for product management sign-off yet, and is still in discussion. The issue summary makes it sound like there's an agreed-upon spec, however.

Either way, it'd be great to have a "future-looking" design that takes into account how Content Moderation options will show up in the new world order.

Berdir’s picture

@webchick: The way I understand, the only thing that content moderation would do is replace the "Published" checkbox with whatever UI it ends up using (a simple select, or some kind of bar or whatever).

I also thought that the only thing that we were blocked on here is making the published widget visual. The suggestion was to move it close to the save button and have it visually separated from the rest of the form.

That is more complicated than it sounds because this is just a standard widget. We could move it a specific place in seven, together with the buttons, but then we lose the ability for site builds to control its position (some might want to have it shown directly under the title or so?) and we also have to keep it configurable, so that it can be switched out with the content moderation widget/UI easily (which seven would also need to support). Since we have no field group concept in core, we can't easily make that configurable.

#103 has some alternative suggestions for the UI.

phenaproxima’s picture

Would it be helpful if I provided a proof-of-concept patch for #103?

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.0-beta1 was released on August 3, 2016, which means new developments and disruptive changes should now be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

ohthehugemanatee’s picture

I have an opinion about the bike shed, too! ;)

We are discussing 3 separate issues here, which do not have to be addressed all at once.

1) UX of a confusing button to "Save and Publish", "Save and Keep Published", etc
2) DX of a UI that limits customization, and loads it with nasty surprises like #2423153: Add menu from the editing page doesn't save the changes.
3) UX of a "published" state widget that is separate from the submit form element.

Issues 1 and 2 are very closely related, and have a simple solution: separate the "save" and "published state" form elements. That's the proposed resolution, and it seems like everyone in this thread agrees.

Issue 3 is actually a problem for all the fundamental node properties like author, revision, and datestamps. Right now these are separate fields, and we use templates and seven.theme to basically magick them into a sidebar. That's not great, and we should consider how to improve it. But we should consider how to improve it for all the fundamental node properties together.

And importantly, we don't have to improve it IN THIS ISSUE. This issue can address 1 and 2 in isolation, following the design pattern already in place for author, revision, and publish date. If we change that design pattern later (in a separate issue), that's OK.

With that in mind, yeah @phenaproxima I think it would be helpful to have a patch to address 1 and 2 per #103, following the design pattern already in place.

alexpott’s picture

One of the funny things about the current pattern is that most of the time you know you want to create a draft or update the published content before you click edit. Forcing the user to choose after is what makes this so tricky. Maybe we should put the choice of editing published node or creating a draft before the choice to edit.

dixon_’s picture

I agree with Alex. It is a little bit strange that the task of editing live content and creating a new draft is only distinguished with a checkbox on the edit form.

Should these two tasks not be a bit more separated? Perhaps separate the two tasks as two different operations, with separate links, tabs etc in the UI? One for "edit live" and one for "create new draft"?

tkoleary’s picture

@alexpott

One of the funny things about the current pattern is that most of the time you know you want to create a draft or update the published content before you click edit. Forcing the user to choose after is what makes this so tricky. Maybe we should put the choice of editing published node or creating a draft before the choice to edit.

Boom. Lateral thinking FTW!

Berdir’s picture

That's an interesting idea, but I don't think it really affects this issue, that's more something to think about for #2753717: Add select field to choose moderation state on entity forms?

If anything, then making this a simple widget instead of a complicated thing with the buttons will make it easier to conditionally hide it, be it always or just in some cases, when the user clicked on "create draft"?

anavarre’s picture

Component: node system » content_moderation.module
Berdir’s picture

Component: content_moderation.module » node system

@anavarre: This issue on its own has nothing to do with content_moderation.module. Other issues for that module depend on it, that's all.

yongt9412’s picture

Duh, did the rebase for 8.2.x by mistake. Just an option we might use a fieldset for this, I added some code for still be able to move it around the form.

Status: Needs review » Needs work

The last submitted patch, 115: change_save_and_keep-2068063-115.patch, failed testing.

yongt9412’s picture

Status: Needs work » Needs review
FileSize
50.33 KB

This time for 8.3.x. Kept the changes as shown in the previous screenshot.

Status: Needs review » Needs work

The last submitted patch, 117: change_save_and_keep-2068063-117.patch, failed testing.

yongt9412’s picture

yongt9412’s picture

Status: Needs work » Needs review
Berdir’s picture

Hm. Not sure about #108. Publication status is a critical part of the content creation workflow and it should IMHO always be visible and not be hidden away. So if we put it somewhere in the sidebar, we should basically put it in the same place as the revision fields, so that they are always visible.

But based on the UX tests we made with the blocks UI, the right sidebar was practically invisible to every single user, not sure if that's better with the node create form?

Using a fieldset to highlight it might work but then we need to work on the wording, because the screenshot looks pretty repetitive. Don't really have any good ideas, though.

Status: Needs review » Needs work

The last submitted patch, 119: change_save_and_keep-2068063-119.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
50.24 KB

Rerolled status is now a standard field, we need to change it.

Berdir’s picture

And here's a version for 8.2 in case anyone needs it.

hass’s picture

How should this fit into content moderation?

The last submitted patch, 123: change_save_and_keep-2068063-123.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 124: change_save_and_keep-2068063-124-8.2.patch, failed testing.

chr.fritsch’s picture

Status: Needs work » Needs review
FileSize
50.09 KB

#123could not be applied. I rerolled it

Status: Needs review » Needs work

The last submitted patch, 128: change_save_and_keep-2068063-128.patch, failed testing.

Berdir’s picture

Well, it applies I think, just has a merge-conflict that I forgot to fix :)

This fixes some tests. Not sure what to do about content_moderation now, we have a bit of a hen/egg problem it seems.

#2753717: Add select field to choose moderation state on entity forms is postponed on this, but content_moderation is messing around with the node save buttons and replacing it with its own similar pattern with a button per moderation_state.

Status: Needs review » Needs work

The last submitted patch, 130: change_save_and_keep-2068063-130.patch, failed testing.

timmillwood’s picture

Status: Needs work » Needs review
FileSize
1.16 KB
55.16 KB

This should fix the failing content_moderation test by just leaving it as a special case. We'll fix the content moderation button in #2753717: Add select field to choose moderation state on entity forms.

timmillwood’s picture

Issue tags: -Needs change record

Short and sweet change record https://www.drupal.org/node/2847274

Status: Needs review » Needs work

The last submitted patch, 132: 2068063-132.patch, failed testing.

timmillwood’s picture

Status: Needs work » Needs review
Issue tags: -Needs upgrade path tests
FileSize
2 KB
56.5 KB

Adding upgrade path test.

Status: Needs review » Needs work

The last submitted patch, 135: 2068063-135.patch, failed testing.

hass’s picture

Issue tags: +content moderation

We should discuss this first with content moderation team as this change may otherwise need to be rolled back for content moderation. Aside there may not enough test coverage as this should break content moderation I guess...

timmillwood’s picture

@hass - As the Content Moderation maintainer I am fine with this. Although you may have a point about test coverage, so tagged this for manual testing.

There is currently some code in \Drupal\content_moderation\Plugin\Field\FieldWidget\ModerationStateWidget that disables Node module's "publish" and "unpublish" buttons, maybe we should remove that from content moderation in this issue? However it could equally be done afterwards in #2753717: Add select field to choose moderation state on entity forms.

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.0-alpha1 will be released the week of January 30, 2017, which means new developments and disruptive changes should now be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Berdir’s picture

Status: Needs work » Needs review
FileSize
56.57 KB

Reroll, simple conflict in a test.

We have been using this for more than a year now on projects and didn't have any issues. One thing I just noticed is that when you hide the checkbox (or when you just applied the patch without running the update or cache clear) then you see the new fieldset but nothing in it.

What we do need might be another UX review. An idea I had was to push it into the sidebar for seven and basically replace the current hardcoded Published/Not Published header at the top. Not sure if that is visible enough.

Status: Needs review » Needs work

The last submitted patch, 140: 2068063-140.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
57.1 KB
1.45 KB

This should fix the fails.

Berdir’s picture

FileSize
19.86 KB
14.62 KB

So, here is how it looks now:

My idea was to replace the static Published text with it somehow in the sidebar:

Status: Needs review » Needs work

The last submitted patch, 142: 2068063-142.patch, failed testing.

Jo Fitzgerald’s picture

Status: Needs work » Needs review
FileSize
57.09 KB
575 bytes

Fix the final test fail.

Status: Needs review » Needs work

The last submitted patch, 145: 2068063-145.patch, failed testing.

Jo Fitzgerald’s picture

Status: Needs work » Needs review

No, it's definitely passing!

timmillwood’s picture

Status: Needs review » Reviewed & tested by the community

I think it's looking pretty good.

hass’s picture

Status: Reviewed & tested by the community » Needs work

This cannot RTBC. See my comments, please.

Berdir’s picture

Your comments have been answered. content_moderation was breaking and has been fixed and has a separate issue to further improve it, which was blocked on this but now no longer is.

That said, we need to get rid of that description at the very least, as I tried to convince @timmillwood in the issue that moved the definition to a separate trait, that's why it re-appeared. I also asked @yoroy for thoughts on my screenshots and this might be a good issue to discuss in the UX meetings.