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: 

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.

Berdir’s picture

Status: Needs work » Needs review
FileSize
353.18 KB

Auto-reroll powered by git.

Berdir’s picture

This patch is better :)

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

Truptti’s picture

Verified the patch '2068063-151.patch' in #152, following are the observation
1.The Patch is applied successfully
2.On applying the patch 'Save and Publish' and 'Save as Unpublished' buttons are replaced and Publish checkbox, Save button,Preview button is displayed while creating a new node
3.On editing a node Publish checkbox, Save button,Preview button and Delete link is displayed.
Attached snapshot 'AfterPatch_Edit_Node.PNG' & AfterPatch_New_Node.PNG' for reference.
This can be marked as RTBC.

Truptti’s picture

Status: Needs review » Reviewed & tested by the community
yoroy’s picture

Status: Reviewed & tested by the community » Needs work

At the very least that "A boolean indicating the published state" description text has to go, it makes things harder to understand instead of helping :)

I'll review this more in depth soon, I wonder if we can be smarter about the visual styling of this.

Berdir’s picture

Yes, I've also noticed problems when you don't have access to the status or if it's configured to not show, then you get an empty publication status wrapper, that definitely looks weird.

@yoroy: Yeah, I agree that we need better visual styling, my idea is in #143 although I quickly tried that and it was surprisingly not easy to get it to where I wanted it. If you want to discuss this in a UX meeting, tell me when and I'll try to join in case there are technical questions.

ifrik’s picture

Status: Needs work » Reviewed & tested by the community

We discussed this in today's UX meeting, and found that this causes a problem when the workflow/content moderation modules are enabled.

The workflow module overrides the default publish button by adding extra statuses. And to complicate matter furthers: users can generate their own statuses, which are then also added to this drop-down button.
(Screenshot and recording to follow.)

It would be good to discuss this issue with people from the Workflow initiative, to see what could be workable solution.

ifrik’s picture

Status: Reviewed & tested by the community » Needs work
Berdir’s picture

@ifrik: Ah, looks like it already was discussed there and I missed that :) the workflow UI is changed in #2753717: Add select field to choose moderation state on entity forms to use a select instead of different buttons. What we need to figure out is in which order those issues need to happen, not sure what is easier. The workflow initiative is definitely aware of this patch and Tim actually worked on it.

another thing to test manually is content translation workflows, which also messes with those buttons.

yoroy’s picture

FileSize
27.78 KB

Video discussion: https://youtu.be/66ZoqccOYPI?t=9m52s

This does need some coordination with the Content Workflow team, because a situation like this does not look like an improvement:

yoroy’s picture

hass’s picture

@Bedir: WTF? That is exactly what I complained about in #149. Tests are also missing as this patch must fail with moderation in mind.

Berdir’s picture

Status: Needs work » Needs review
FileSize
61.74 KB
4.91 KB

As discussed, this implements field access for status field when moderation is enabled, incuding test coverage. Also fixed up the published status thing to be translated, optional and a details element not a fieldset. Still not convinced by this pattern at all, but at least it is technically working now and we have something that can properly be reviewed and discussed by the UX team.

@hass: What you said is that this should be discussed with the moderation team. It was not just discussed, @timmillwood, which is part of that team worked on this issue. And no, it doesn't fail, because the manually selected status is simply irrelevant and overriden when using moderation states. So it was technically working as expected, just resulted in a confusing UI, which is now fixed.

#2753717: Add select field to choose moderation state on entity forms will then also get rid of the dropbutton for the workflows and replace it with a simple select instead.

timmillwood’s picture

Issue tags: -Needs manual testing
  1. +++ b/core/modules/node/src/NodeForm.php
    @@ -145,35 +145,25 @@ public function form(array $form, FormStateInterface $form_state) {
    +      '#type' => 'details',
    

    I prefered this as a fieldset rather than details.

  2. +++ b/core/modules/node/src/NodeForm.php
    @@ -145,35 +145,25 @@ public function form(array $form, FormStateInterface $form_state) {
    +      '#title' => $this->t('Published status'),
    

    I wonder if "Publishing status" reads better than "Published status"

Berdir’s picture

1. fieldsets don't support #optional which is why I changed it. I don't like either fieldset or details, actually :) The labels are duplicated either way. As mentioned before, my idea is putting it in the same place as the revision checkbox/field, where we have a static string for published/unpublished right now when editing.

timmillwood’s picture

I have updated the patch to move the checkbox to the "meta" area where the <h3> Published</h3> currently is. This is done in seven.theme, so I also made it look nice without seven. Then if the status field doesn't exist or the user doesn't have access to it the old <h3> is used.

Bartik - Published node

Bartic - Unpublished node

Seven

Seven - With content moderation

Berdir’s picture

Nice. As mentioned in IRC, maybe we need a bit of styling in seven to make that bigger/more visible, lets see what the UX team thinks about it.

Also, an idea that I had is that with workflows, it might make sense if seven would display the current workflow state there and not published/unpublished, but that's obviously not something for this issue.

timmillwood’s picture

Right, adding the following to seven's CSS will change the label to have the same styling as a the old <h3> tag, I'll see what @yoroy thinks before updating the patch.

.form-item-status-value label {
    font-weight: bold;
    font-size: 1.231em;
}
kattekrab’s picture

@yoroy #156

At the very least that "A boolean indicating the published state" description text has to go, it makes things harder to understand instead of helping :)

I have an issue for that one. With a patch!!

#2849357: Remove "Published" status description to align with "Promoted" and "Sticky at top of lists"

Berdir’s picture

@kattekrab: This issue already removes the description as well and it changes the label to Published, but just for node. I would suggest we update this patch here accordingly and close yours as a duplicate.

kattekrab’s picture

Sounds good to me @Berdir - I'll do that now.

yoroy’s picture

@berdir is there a rationale for why we should consider moving the "published" checkbox to the sidebar?

Currently you can create, edit, publish, unpublish content while ignoring all the stuff in the sidebar. Moving the checkbox for publishing status to the sidebar would break that.

It's also not very comfortable to see the setting for published state move to different locations on the page depending on which modules you enable. That makes things unpredictable on a very important screen where we should strive to be consistent so that we don't undermine people's confidence.

timmillwood’s picture

@yoroy - The sidebar is only for Seven.theme, so we can move it out very easily, but by default (in Bartik for example) should it be one of the vertical tabs or just a fieldset above (or below) that?

Berdir’s picture

It was just an idea I had. The main reason is that we currently have that static info there about Published/Not Published, so I wanted to try how it looks when the checkbox is there and get your feedback on that.

I'm perfectly fine not moving it there. That said, when we don't do that, I'd rather not have a fieldset/details element at all, because that IMHO looks strange and very repetitive.. I'd rather do something in seven.theme that moves it into a common container with the save buttons so that we can have a visual separation there as you suggested a while ago.

The downside of doing anything (fieldset, moving to sidebar or save buttons) is that we break the ability for users to position the checkbox where they want it, as they can with other fields. For example, someone might want to have that checkbox directly below/above the title.

yoroy’s picture

Thanks berdir.

I'd rather do something in seven.theme that moves it into a common container with the save buttons so that we can have a visual separation there as you suggested a while ago.

yes!

The downside of doing anything (fieldset, moving to sidebar or save buttons) is that we break the ability for users to position the checkbox where they want it, as they can with other fields.

Would such a change make that *impossible* to do or would it be *more work* to make it look right when moving to another location on the page?

Berdir’s picture

Depends on where we make the change. If we do it in node.module then it is there for everyone and it is relatively easy to change it with a custom module that reverts those changes.

If we make it seven specific then another them would simply not have it, so if you have another admin theme you can dowhat you want, but if you do use seven.theme then it is very hard to undo that change as seven is the last to alter that form.

timmillwood’s picture

I'm wondering if we're best going back to the patch in #164.

Berdir’s picture

Possibly. Rerolled #164, will think about what to do with seven.

tim.plunkett’s picture

  1. +++ b/core/modules/content_moderation/content_moderation.module
    @@ -15,9 +15,12 @@
    +use Drupal\node\Plugin\views\filter\Access;
    ...
     use Drupal\node\NodeInterface;
     use Drupal\node\Plugin\Action\PublishNode;
    

    This looks out of place. Also, the module doesn't depend on node.module, is this okay?

  2. +++ b/core/modules/menu_ui/menu_ui.module
    @@ -352,11 +352,7 @@ function menu_ui_form_node_form_alter(&$form, FormStateInterface $form_state) {
    -  foreach (array_keys($form['actions']) as $action) {
    -    if ($action != 'preview' && isset($form['actions'][$action]['#type']) && $form['actions'][$action]['#type'] === 'submit') {
    -      $form['actions'][$action]['#submit'][] = 'menu_ui_form_node_form_submit';
    -    }
    -  }
    +  $form['actions']['submit']['#submit'][] = 'menu_ui_form_node_form_submit';
    

    yay!

  3. +++ b/core/modules/menu_ui/src/Tests/MenuNodeTest.php
    @@ -13,7 +13,6 @@
     class MenuNodeTest extends WebTestBase {
    -
       /**
    

    Out of scope, and also wrong

  4. +++ b/core/modules/node/node.install
    @@ -253,5 +254,28 @@ function node_update_8301() {
    +  $query = \Drupal::entityQuery('entity_form_display')
    +    ->condition('targetEntityType', 'node');
    +  $ids = $query->execute();
    ...
    +    ])
    +      ->save();
    

    Weird indentation choices.

  5. +++ b/core/modules/node/src/NodeForm.php
    @@ -183,59 +173,6 @@ protected function actions(array $form, FormStateInterface $form_state) {
    -    if ($element['submit']['#access'] && \Drupal::currentUser()->hasPermission('administer nodes')) {
    -      // isNew | prev status » default   & publish label             & unpublish label
    -      // 1     | 1           » publish   & Save and publish          & Save as unpublished
    -      // 1     | 0           » unpublish & Save and publish          & Save as unpublished
    -      // 0     | 1           » publish   & Save and keep published   & Save and unpublish
    -      // 0     | 0           » unpublish & Save and keep unpublished & Save and publish
    

    Thank goodness!

timmillwood’s picture

FileSize
1.99 KB
61.33 KB

Re-roll of #180 and fixes for items mentioned in #181.

@tim.plunkett - Re: #181.1 this is not really ok, although there are some Node specific things needed even though Content Moderation doesn't depend on Node, although this is out of scope for this issue, there are a couple of other issues looking into these things.

Status: Needs review » Needs work

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

Berdir’s picture

Status: Needs work » Needs review
FileSize
62.02 KB
1.9 KB

Fixed that test and changed the published status label directly in the trait instead of just for node.

Munavijayalakshmi’s picture

+++ b/core/modules/book/tests/src/Functional/BookTest.php
@@ -741,8 +741,8 @@ public function testBookNavigationBlockOnUnpublishedBook() {
+    $edit = array('status[value]' => FALSE);

+++ b/core/modules/file/src/Tests/FileFieldWidgetTest.php
@@ -121,8 +121,8 @@ public function testSingleValuedWidget() {
+      $node_storage->resetCache(array($nid));

+++ b/core/modules/node/src/Entity/Node.php
@@ -405,6 +405,16 @@ public static function baseFieldDefinitions(EntityTypeInterface $entity_type) {
+    $fields['status']
+      ->setDisplayOptions('form', array(
+        'type' => 'boolean_checkbox',
+        'settings' => array(
+          'display_label' => TRUE,
+        ),
+        'weight' => 120,
+      ))

+++ b/core/modules/node/src/Tests/NodeEditFormTest.php
@@ -124,12 +124,21 @@ public function testNodeEdit() {
+    $this->nodeStorage->resetCache(array($node->id()));

use short array syntax (new coding standard).

Fixed the short array syntax error and attached new patch.

timmillwood’s picture

FileSize
1.13 KB
62.03 KB
22.5 KB

After talking about this on yesterday's UX meeting and talking with @jojototh today we came up with this solution:

The last submitted patch, 186: interdiff-2068063-186.patch, failed testing.

Berdir’s picture

I still find it a strange to have that duplicated label, the proposal back in #94 didn't have that and that looks better to me.

I would actually also suggest to move the markup into the template, we already have node-edit-form.html.twig which has three regions, then we could just display status in the footer and put the hr above in there?

timmillwood’s picture

Removed the label, like in #94. I think it seems a bit bare now.

Also moved the <hr/> to the twig template, but note, this is in classy, therefore admin themes that extend this will just get the checkbox.

I also removed the field from being display configurable because it doesn't make sense.

Berdir’s picture

It IMHO still makes sense to be configurable IMHO, you could for example hide it if you manage it somehow. And it makes sense for themes that do not extend from classy. We have similar things with e.g. the node links, which can also be moved around but they are in a fixed position in the default template.

Wrong screenshot, that still has the label?

Worth mentioning that the line will now always be there, also if access to status is denied. Not sure if that's a feature or a bug. Would be nice to have a screenshot with the default look in seven and when you are using workflows? And possibly as a user without administer nodes permission? I could also imagine that we'd want the upcoming workflows select to also show up below that line, but I guess that's something to figure out in the other issue and trickier, as it is now an always existing base field.

timmillwood’s picture

Yeh, I guess no hard in making it configurable.

Uploaded the right screenshot, but embedded the wrong one, edited.

I guess the last paragraph in #190 shows the downsides of #189 compared to #186. The patch in #186 has the <hr/> as part of the field, so if the field is removed intentionally, via a form alter, or due to permissions, the <hr/> is gone too.

timmillwood’s picture

Issue tags: +Needs usability review

Status: Needs review » Needs work

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

Manuel Garcia’s picture

Sorry to be late to the party, here's my 2 cents when it comes to usability:

I feel that the published checkbox is a bit confusing for content authors, since publishing / unpublishing is a user action in their head. Having a button specifically for toggling the published state would be more in line with the concept of performing an action.

How about having three distinct (separate) buttons?

  1. Save
  2. Save and publish (or unpublish, depending on the current published status of the content)
  3. Preview
timmillwood’s picture

@Manuel Garcia - My only concern there is, what does "save" do? I guess it's need to be "save and keep published", "save and unpublish" as the two options, which is what we have now, but just in a drop button. So I guess we could move them out of a drop button into separate buttons.

Manuel Garcia’s picture

@timmillwood good point - yeah then doing that I think would be a clearer path for the user than having a checkbox for it in my opinion.

timmillwood’s picture

Status: Needs work » Needs review

I've been looking at what other systems do.

It seems like when I spun up a site on wordpress.com there was no way to save as unpublished, but a bit of googling seems to suggest it's possible to get "publish" and "save draft" buttons.

Joomla does pretty much what we're suggesting in #2753717: Add select field to choose moderation state on entity forms, a save button, then a select list to choose published, unpublished, archived etc.

Perch (https://grabaperch.com/features/use-bootstrap-foundation) has a "save as draft" checkbox, so I assume not checking this and clicking save will publish.

Wagtail, a django CMS by Drupal shop Torchbox (https://wagtail.io/) has a button similar to our current drop button.

Silver stripe (https://www.silverstripe.org/) has multiple buttons as @Manuel Garcia is suggesting.

Typo3, couldn't work it out.

yoroy’s picture

It's good to keep in mind that the dropbutton was introduced because earlier usability testing showed that people were unsure about what "Save" would do. This was probably for Drupal 6, where the "Published" checkbox was not close to the "Save" button, tucked away in a collapsed fieldset underneath.

I *think* with the published checkbox right above the Save button we're good and won't reintroduce that particular problem.

To recap our options:

With the individual buttons for both we will have to consider the label variations for unpublished to unpublished, published to unpublished, unpublished to published, unpublished to unpublished. And then we'd probably arrive at the forced wording we use in the dropbutton now, but with 2 very wide buttons.

jonathan1055’s picture

Been lurking here and watching this thread for ages, now its time I posted.

My opinion is that the checkbox option is by far the best:

  1. it keeps the buttons simple and short
  2. it keeps the buttons consistent - you do not have to read the changing words to work out what is going to happen
  3. everyone knows what 'save' does. It saves your work. The 'save' button is common to so many software applications
  4. after only a minimal amount of usage the checkbox will become so obvious and clear, that it will be easy to see immediately if the content is going to be published or not
  5. the checkbox will show the existing published state, so if you have not changed it you will know you are not going to alter the state. Having two buttons, one of which will change the state but both of which have 'save' in the text is asking for mistakes to happen
  6. I also like the label for 'publishing status' as shown in #186 as this emphasises the checkbox as being an important part of the form and the flow. Yes it kind of doubles the wording but that's fine for this vital piece of information

Jonathan

yoroy’s picture

yoroy’s picture

We discussed this extensively again at our UX meeting at devdays Seville (link to follow).

We concluded that we want to go for the checkbox option. Individual buttons would become potentially very long when translations are involved. In Seven theme we do show the current publishing status but in another location (top of the right sidebar). We decided for now to leave it there. Showing the current publishing status is sufficiently covered by the label of the checkbox itself.

timmillwood’s picture

So... now that's decided.

<hr/> like it?
should it be in node-edit-form.html.twig of classy or part of the form element in NodeForm.php?
If the site builder disabled the form element via "Manage form display" should the <hr/> also be removed?

tkoleary’s picture

No. It provides a clean separation for the submits which is visually helpful in all cases.

Bojhan’s picture

Thanks for the decision, sounds great! We do have to consider if this is a 8.4 thing, given that it changes a core interaction of a 99% field? It will need a big change notice.

@tkoleary I assume that you can remove the form submission element? Then I guess the HR is not needed, but I dont get the point of removing the form submussion element here :)

lauriii’s picture

Thanks @yoroy for pointing out the question on #202. I think using the node-edit-form.html.twig is sufficient. As a first step, you could add this only into Seven theme, not in Classy or Stable.

I think it is unlikely that we would like to make an exception on this one since it could potentially be disruptive. However, I will discuss with @Cottser and we will make a decision based on that.

timmillwood’s picture

FileSize
2.38 KB
62.9 KB

Making the form display configurable again as suggested in #190, and moving the <hr/> to Seven as suggested in #205.

The biggest remaining issue now is the <hr/> is still there is the field is disabled or the user doesn't have access. Is this ok? if not how do we remove it in Twig?

tkoleary’s picture

The biggest remaining issue now is the


is still there is the field is disabled or the user doesn't have access. Is this ok? if not how do we remove it in Twig?

IMO it's not a problem.

tkoleary’s picture

And BTW this may seem like a small change but this is so much better now. Many many authors will be happy.

xjm’s picture

Hm I'm less confident about that <hr> than @tkoleary. Also it should be possible to conditionally display it in Twig, no? Not sure whether that's what we want or not but it seems like we should not add unnecessary markup to the page when it's not useful. Also, are we sure we should do this with markup vs. CSS?

Agreed on #205 FWIW.

xjm’s picture

Oh and re: #204, yes, this is definitely a minor-only change (not backportable to 8.3.x after alpha IMO since it changes basically the most important UI). There is already a change record draft attached to the issue.

Bojhan’s picture

Can we not just add this in Seven?

lauriii’s picture

Issue tags: +accessibility

Tagging for accessibility based on #209.

timmillwood’s picture

@Bojhan - what's "this"? the whole checkbox? the hr?

@xjm - displaying conditionally in Twig is beyond me, I tried {% if form.status %}<hr/>{% endif %} but that didn't seem to work. I guess form.status could be set without the field being rendered? I don't know. Maybe a CSS border top on the field might work better, then it would only appear if the field is rendered? We can have a try.

andrewmacpherson’s picture

I saw the accessibility tag was added re: #209. Is this a question about accessibility of <hr> versus a CSS border? I read the comments as far back as #188 where the separator was first introduced.

On balance, if we do go for a horizontal rule, make it a real semantic <hr>. Here's what I know about assistive tech support:

  • Screen readers can announce a <hr>, but it varies a lot.
  • Some screen readers provide a way to skip to the next separator, similar to how you can navigate by headings. JAWS and NVDA do, ChromeVox and VoiceOver do not, AFAIK.
  • You won't get these benefits from a CSS border.
  • It's possible to label a <hr>. I made a test page with HR elements labeled in various ways. I found that ChromeVox and Talkback will announce a separator, but only if it has a label. I need to test it with more screen readers. I don't know how helpful this is, what the label would say, or whether this is better than a heading.

The most pertinent comment is this one from @tkoleary in #203:

No. It provides a clean separation for the submits which is visually helpful in all cases.

I like this idea, though surely it applies to forms on all admin pages? The patch in #206 only adds a separator to the node form. Preceding the submit buttons with a separator, but just on node forms, kind of goes against the spirit (if not the letter) of WCAG SC 3.2.4 - Consistent Identification.

Bojhan’s picture

@tim Yep, the <hr>

timmillwood’s picture

@Bojhan - in #206 the <hr> was added to Seven via core/themes/seven/templates/node-edit-form.html.twig

It sounds like from #214 a <hr> is better than a CSS border top.

jonathan1055’s picture

@andrewmacpherson in #214

I like this idea, though surely it applies to forms on all admin pages? The patch in #206 only adds a separator to the node form. Preceding the submit buttons with a separator, but just on node forms, kind of goes against ...

AFAIK The <hr> is not intended to be added above the submit buttons, it is meant to be above the checkbox so that the checkbox does not get lost. The main discussion now is how to remove the <hr> when there is no checkbox. However, I agree with @tkoleary that it's not a problem if we have a horizontal rule when the checkbox is not there.

timmillwood’s picture

So even after all this discussion I think the patch in #206 might be good to go from a UX and accessibility point of view? So could do with a code review.