Problem/Motivation

In #2068063: Change "Save and keep un-/published" buttons to a "Published" checkbox and an included "Save" button there was a lot of discussion on the UI for saving nodes. The media module originally copied what the node modules does and should follow the same pattern once #2068063: Change "Save and keep un-/published" buttons to a "Published" checkbox and an included "Save" button lands.

Proposed resolution

Apply the same UI changes from #2068063: Change "Save and keep un-/published" buttons to a "Published" checkbox and an included "Save" button to the media module.

Remaining tasks

User interface changes

API changes

Data model changes

With core and media as-is:

With content moderation added on top:

With content moderation + #2753717: Add select field to choose moderation state on entity forms applied:

CommentFileSizeAuthor
#44 2863431-44.patch13.56 KBManuel Garcia
#44 interdiff.txt776 bytesManuel Garcia
#42 Edit File reyery erty try | drupal 8.4.x 2017-07-10 16-30-29.png45.41 KBGábor Hojtsy
#41 Edit File my file | drupal 8.4.x 2017-07-10 16-05-54.png32.77 KBGábor Hojtsy
#37 Screenshot from 2017-07-10 10-15-41.png18.36 KBManuel Garcia
#37 2863431-37.patch13.29 KBManuel Garcia
#37 interdiff.txt501 bytesManuel Garcia
#35 2863431-35.patch13.29 KBManuel Garcia
#35 interdiff.txt349 bytesManuel Garcia
#33 2863431-33.patch13.29 KBManuel Garcia
#27 2863431-27.patch13.15 KBManuel Garcia
#27 interdiff.txt626 bytesManuel Garcia
#23 screen-wide-2863431-21.png174.61 KBnaveenvalecha
#23 screen-small-2863431-21.png.png93.01 KBnaveenvalecha
#21 2863431-21.patch13.15 KBManuel Garcia
#21 interdiff-15-21.txt3.61 KBManuel Garcia
#21 interdiff.txt633 bytesManuel Garcia
#18 media-add-small.png19.87 KBManuel Garcia
#18 media-add-widescreen.png21.9 KBManuel Garcia
#18 2863431-18.patch12.87 KBManuel Garcia
#18 interdiff.txt3.32 KBManuel Garcia
#16 Screen Shot 2017-06-19 at 11.51.13 AM.png140.38 KBnaveenvalecha
#16 Screen Shot 2017-06-19 at 11.50.18 AM.png177.42 KBnaveenvalecha
#15 interdiff-11-15.txt1.01 KBseanB
#15 2863431-15.patch11 KBseanB
#12 interdiff-9-11.txt3.38 KBseanB
#11 2863431-11.patch9.62 KBseanB
#9 2863431-9-do-not-test.patch5.74 KBManuel Garcia
#9 2863431-including-2068063-279.patch70.09 KBManuel Garcia
#9 interdiff.txt752 bytesManuel Garcia
#7 interdiff-5-7.txt717 bytesseanB
#7 2863431-7-do-not-test.patch5.59 KBseanB
#7 2863431-7-including-2068063-264.patch69.46 KBseanB
#5 2863431-5-do-not-test.patch5.59 KBseanB
#5 2863431-5-including-2068063-264.patch69.46 KBseanB
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

seanB created an issue. See original summary.

xjm’s picture

Berdir’s picture

Note that there is no technical reason to postpone in on the resolution of that issue, we AFAIK know what we do there, we just need to do the same here, there is no code dependency, just a dependency on the decision and so on.

But it is obviously postponed on the media patch :)

naveenvalecha’s picture

Status: Postponed » Active
seanB’s picture

First attempt.

The last submitted patch, 5: 2863431-5-including-2068063-264.patch, failed testing.
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

seanB’s picture

Yes, the status checkbox was obviously named differently.

Berdir’s picture

Status: Needs review » Needs work
+++ b/core/modules/media/src/MediaForm.php
@@ -61,47 +59,6 @@ protected function actions(array $form, FormStateInterface $form_state) {
     $element['delete']['#access'] = $media->access('delete');
     $element['delete']['#weight'] = 100;
 

the parent already checks access for the delete button and I think the weight was also only necessary because we added additional buttons. So we can then remove access() completely.

It's possible this is true for NodeForm as well although there we possibly need to think more about BC and other buttons added by contrib. Less of a problem here.

Manuel Garcia’s picture

Status: Needs work » Needs review
FileSize
752 bytes
70.09 KB
5.74 KB

Removing MediaForm::actions() as per #8.
Combined patch now with the latest patch on #2068063: Change "Save and keep un-/published" buttons to a "Published" checkbox and an included "Save" button.

Berdir’s picture

Status: Needs review » Needs work

Looks good to me now, just needs a reupload of the patch without the other one to make sure that passes on its own. And some manual testing.

seanB’s picture

Status: Needs work » Needs review
FileSize
9.62 KB

Since the file and image plugins are committed had to replace some more. New patch attached.

seanB’s picture

FileSize
3.38 KB

Forgot the interdiff. Not that exciting, but still.

Status: Needs review » Needs work

The last submitted patch, 11: 2863431-11.patch, failed testing. View results

Manuel Garcia’s picture

Issue tags: +Needs manual testing
seanB’s picture

Status: Needs work » Needs review
FileSize
11 KB
1.01 KB

Forgot to add the status field to the image/file media types.

naveenvalecha’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs manual testing
FileSize
177.42 KB
140.38 KB

Nice. Patch looks good to me.
Also done the Manual testing and it looks good on widescreen and small screen.

Wide screen
Widescreen Media Add form

Small Screen
small screen Media Add form

//Naveen

tstoeckler’s picture

Status: Reviewed & tested by the community » Needs work

Looking pretty sweet. Have to kick this back though, but I think we're close.

  1. +++ b/core/modules/media/config/install/core.entity_form_display.media.image.default.yml
    @@ -34,6 +34,13 @@ content:
    +    weight: 120
    
    +++ b/core/modules/media/src/Entity/Media.php
    @@ -423,6 +423,16 @@ public static function baseFieldDefinitions(EntityTypeInterface $entity_type) {
    +        'weight' => 120,
    

    Seems that's a bit high, if it's below the Save button (per the screenshots in #16 (thanks @naveenvalecha!)), no?

  2. +++ b/core/modules/media/src/MediaForm.php
    @@ -49,66 +49,10 @@ public function form(array $form, FormStateInterface $form_state) {
    -    $form['#entity_builders']['update_status'] = [$this, 'updateStatus'];
    -
    

    The function itself should be removed as well.

Manuel Garcia’s picture

Status: Needs work » Needs review
FileSize
3.32 KB
12.87 KB
21.9 KB
19.87 KB

Re #17.1:
'author' form field is being set with a weight of 90, so I've set the published checkbox to have a weight of 100 which gets it between the authorship and the submit button.

I noticed this happened also on the file form so fixed that there as well.

Re #17.2:
Removed MediaForm::updateStatus, good catch.

I noticed we also had the same layout issue as we ran into when we need the node form, so I've added a fix for it to the seven theme, using essentially the same approach. This is to do with the space between the published checkbox and the elements above and below. See screenshots to see how it looks with this patch.

Widescreen:
Media add widescreens screenshot

Small screens:
Media add small screens screenshot

tstoeckler’s picture

Status: Needs review » Needs work
+++ b/core/themes/seven/seven.libraries.yml
@@ -119,3 +119,11 @@ tour-styling:
+      css/layout/media-add.css: {}

I think you forgot to add the CSS file to the patch. Other than that looks great, and great catch regarding the layout issue!

naveenvalecha’s picture

#17.2 Nice catch.

+++ b/core/themes/seven/seven.libraries.yml
@@ -119,3 +119,11 @@ tour-styling:
+media-form:

We had a discussion earlier in some issue where we moved the media configuration from standard profile to media module itself, to keep all the media related configuration in the media itself. I'm not sure we could do the change in the seven theme itself. Probably a core committer could help us.

Manuel Garcia’s picture

Status: Needs work » Needs review
FileSize
633 bytes
3.61 KB
13.15 KB

Re #19: thanks! yeah forgot to add the CSS file, here is a new patch with it in it.
I also realized the library dependency was incorrect, so fixing this now (see interdiff.txt).

Re #20: thanks for the heads up... this isn't configuration so perhaps we can just add it to seven theme...
also, I'm not sure how else we could do this if that's not the case :-s

The last submitted patch, 18: 2863431-18.patch, failed testing. View results

naveenvalecha’s picture

Verified on my local. It addressed #17.1 and #17.2. See the screenshots below
However, I'm not sure about the #20 so leaving it for the core committers, marking it RTBC to know their opinion.

Media add wide screen

wide

Media add small screen

small

//Naveen

Gábor Hojtsy’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/modules/media/tests/src/FunctionalJavascript/MediaSourceImageTest.php
--- /dev/null
+++ b/core/themes/seven/css/layout/media-add.css

+++ b/core/themes/seven/css/layout/media-add.css
+++ b/core/themes/seven/css/layout/media-add.css
@@ -0,0 +1,3 @@

@@ -0,0 +1,3 @@
+.media-form .field--name-status {
+  margin-top: 1.5em;
+}

The naming of the file looks odd to me because this CSS is / should be used on media editing as well, not just adding. Also I did not find a similar entry for the node form, neither as a separate CSS file, nor in an existing CSS file. I only found mentions of field--name-status in the bartik form.css and not nearly with this styling.

What is this based off of? Can we fix the filename? Why is it in its own file?

Manuel Garcia’s picture

Re: #24

Its based on core/themes/seven/css/layout/node-add.css (on the same folder). Only on the node form this is tackled via its container, .layout-region-node-footer__content (because we had to put a horizontal line there, see #2068063: Change "Save and keep un-/published" buttons to a "Published" checkbox and an included "Save" button).

I agree perhaps we should name both files node-form.css and media-form.css, just followed what was there. I've put it in its own file because you only need it if media module is enabled.

tacituseu’s picture

@Manuel Garcia:
core/themes/seven/css/layout/node-add.css is an old overlay artefact, getting rid of it in #2844714: Remove obsolete overlay styles and supporting logic in seven_preprocess_html().

Edit:
Not likely to just remove it now as #2068063: Change "Save and keep un-/published" buttons to a "Published" checkbox and an included "Save" button modified it.

Manuel Garcia’s picture

Status: Needs work » Needs review
FileSize
626 bytes
13.15 KB

Thanks @tacituseu for the information, I had no idea.

Where else should we put these styles then? I can see that in the directory core/themes/seven/css/components/ there are files for specific modules among other things, for example node.css:

.node__submitted {
  margin: 1em 0;
}

One file that looks like a candidate in that folder is form.css, but it seems to be only for generic form elements (no module specific styles there).

Can we add this as media.css to that folder? This patch does this in case it'll work for you guys. Else please let me know where else to put this and I'll happily prepare another patch =)

Gábor Hojtsy’s picture

Given #2753717: Add select field to choose moderation state on entity forms, I think the media form would equally need the border as well to make the UI clear. See how that uses the border to designate the publication status area from the rest of the form.

timmillwood’s picture

In NodeForm we add the footer element, which is what the border is applied to, maybe we should be adding this to \Drupal\Core\Entity\ContentEntityForm::form for all entity types that implement EntityPublishedInterface, the move the CSS as well to a generic place. We would have to test how this would affect \Drupal\comment\CommentForm because Comment entity implements EntityPublishedInterface

phenaproxima’s picture

Issue tags: -D8Media +Media Initiative

Re-tagging.

marcoscano’s picture

Status: Needs review » Needs work

Updating the status to address the border requested in #28, and also:

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

It appears that this weight 120 here will send the checkbox back to after the "Save" button.

I suggest we open a follow-up to implement a generic solution for the border as suggested in #29, so we keep this issue restricted to the media scope.

Manuel Garcia’s picture

Re #29: actually the border is applied to .layout-region-node-footer__content, which is a css class living on core/themes/seven/templates/node-edit-form.html.twig (A template we added on #2068063: Change "Save and keep un-/published" buttons to a "Published" checkbox and an included "Save" button).

If I remember correctly, the footer form element was necessary in order to be able to print it just above the form actions on the template, and to remove the margin on Bartik from the published checkbox, which was showing misaligned due to general form element styles getting applied.

So in essence, moving the whole "border top published checkbox" functionality to all contententity forms is a bit more involved, we'd probably have to introduce a generic template for these if I'm not mistaken...

In any case, I have created the issue to explore this posibility, please have a look: #2892304: Introduce footer region to ContentEntityForm

Manuel Garcia’s picture

Status: Needs work » Needs review
FileSize
13.29 KB

Reroll of #27, there were conflicts on:

  • core/modules/media/src/MediaForm.php
  • core/modules/media/tests/src/Functional/MediaRevisionTest.php
Wim Leers’s picture

I really like the diffstat: 62 insertions, 90 deletions!

I don't know the mechanics of this well enough to provide a helpful review unfortunately.

Manuel Garcia’s picture

Updating the seven library dependency as #2887269: Remove unnecessary prefixing from media libraries got in.

phenaproxima’s picture

Status: Needs review » Reviewed & tested by the community

Welp, looks pretty good to me!

Manuel Garcia’s picture

@marcoscano good catch on the weight issue, you were spot on. Addressing that (#31) on this patch.

Published checkbox media

I feel its ok to leave as RTBC, being this such a small change =)

Manuel Garcia’s picture

timmillwood’s picture

+++ b/core/modules/media/tests/src/FunctionalJavascript/MediaSourceImageTest.php
@@ -62,7 +62,7 @@ public function testMediaImageSource() {
diff --git a/core/themes/seven/css/components/media.css b/core/themes/seven/css/components/media.css

diff --git a/core/themes/seven/css/components/media.css b/core/themes/seven/css/components/media.css
new file mode 100644

new file mode 100644
index 0000000..34b1599

index 0000000..34b1599
--- /dev/null

--- /dev/null
+++ b/core/themes/seven/css/components/media.css

+++ b/core/themes/seven/css/components/media.css
+++ b/core/themes/seven/css/components/media.css
@@ -0,0 +1,3 @@

@@ -0,0 +1,3 @@
+.media-form .field--name-status {
+  margin-top: 1.5em;
+}
diff --git a/core/themes/seven/seven.libraries.yml b/core/themes/seven/seven.libraries.yml

diff --git a/core/themes/seven/seven.libraries.yml b/core/themes/seven/seven.libraries.yml
index 424e7f5..21ac5bc 100644

index 424e7f5..21ac5bc 100644
--- a/core/themes/seven/seven.libraries.yml

--- a/core/themes/seven/seven.libraries.yml
+++ b/core/themes/seven/seven.libraries.yml

+++ b/core/themes/seven/seven.libraries.yml
+++ b/core/themes/seven/seven.libraries.yml
@@ -119,3 +119,11 @@ tour-styling:

@@ -119,3 +119,11 @@ tour-styling:
   css:
     theme:
       css/components/tour.theme.css: {}
+
+media-form:
+  version: VERSION
+  css:
+    layout:
+      css/components/media.css: {}
+  dependencies:
+    - media/form
diff --git a/core/themes/seven/seven.theme b/core/themes/seven/seven.theme

diff --git a/core/themes/seven/seven.theme b/core/themes/seven/seven.theme
index 91c03e6..bea2465 100644

index 91c03e6..bea2465 100644
--- a/core/themes/seven/seven.theme

--- a/core/themes/seven/seven.theme
+++ b/core/themes/seven/seven.theme

+++ b/core/themes/seven/seven.theme
+++ b/core/themes/seven/seven.theme
@@ -186,3 +186,10 @@ function seven_form_node_form_alter(&$form, FormStateInterface $form_state) {

@@ -186,3 +186,10 @@ function seven_form_node_form_alter(&$form, FormStateInterface $form_state) {
   $form['revision_information']['#type'] = 'container';
   $form['revision_information']['#group'] = 'meta';
 }
+
+/**
+ * Implements hook_form_BASE_FORM_ID_alter() for \Drupal\media\MediaForm.
+ */
+function seven_form_media_form_alter(&$form, FormStateInterface $form_state) {
+  $form['#attached']['library'][] = 'seven/media-form';
+}

I'm not really a fan of this CSS, as it won't be needed once we have a more generic solution. Gabor pointed out that Seven theme is @internal so we can rip it out, but maybe we need a @todo in here pointing to the follow up?

Gábor Hojtsy’s picture

Status: Reviewed & tested by the community » Needs work

Let's add the @todo. Would #2892304: Introduce footer region to ContentEntityForm rip it out?

Also anyone tested this with #2753717: Add select field to choose moderation state on entity forms? Tim Millwood on IRC said he did and there seem to be some weight issues with it, but generally should be compatible. Now that patch is not yet in core, so technically that should fix whatever it needs to work with this but if it is really about just fixing a weight here, then why not? :)

Gábor Hojtsy’s picture

Tim and I both tested the form with the current content moderation module, and it looks like this:

Also added to the issue summary. (The goal of this patch was not and cannot be to "fix" the same UI in content moderation, but it should still work with content moderation -- and it does).

Gábor Hojtsy’s picture

Just tested with #2753717: Add select field to choose moderation state on entity forms too and it seems to be all right (other than the horizontal line would be nice to have here, but that is for #2892304: Introduce footer region to ContentEntityForm). Screenshot:

Also added to the issue summary. I don't see a problem with this, so I think adding the todo as per @timmillwood sounds like the only missing step.

Manuel Garcia’s picture

Thanks all,
#2892304: Introduce footer region to ContentEntityForm at this point would not rip out this CSS, since the layout changes we made to the node form when we removed these buttons on #2068063: Change "Save and keep un-/published" buttons to a "Published" checkbox and an included "Save" button do not apply to Media unfortunately - these included CSS and a new node-edit-form.html.twig template. The new template is specially important in this.

In order to provide the "line on top" of the status field, all ContentEntityForm's would have to use the same template, which is something I think worth exploring on #2892304: Introduce footer region to ContentEntityForm.

+++ b/core/modules/media/src/Entity/Media.php
@@ -424,6 +424,16 @@ public static function baseFieldDefinitions(EntityTypeInterface $entity_type) {
+    $fields['status']
+      ->setDisplayOptions('form', [
+        'type' => 'boolean_checkbox',
+        'settings' => [
+          'display_label' => TRUE,
+        ],
+        'weight' => 100,
+      ])
+      ->setDisplayConfigurable('form', TRUE);
+

This would be currently ripped out by #2892304: Introduce footer region to ContentEntityForm.

I hate to say it but... should postpone on #2892304: Introduce footer region to ContentEntityForm?

Manuel Garcia’s picture

Status: Needs work » Needs review
FileSize
776 bytes
13.56 KB

OK looks like its been decided that #2892304: Introduce footer region to ContentEntityForm will not be adding the field to the form, so updating the status field base definition will be necessary here as well.

I've also had a bit of an adventure in defining a single template for all these entities but have decided against it because these can all serve very different purposes so we should not try optimizing too early. So in my opinion in order for the media form to get the line above the published checkbox in the same manner as node, it will have to define its own media-edit-form.html.twig, once #2892304: Introduce footer region to ContentEntityForm gets in (which defines the footer region). So no need to postpone.

Adding the @todo for this and revisiting the media css.

seanB’s picture

Status: Needs review » Reviewed & tested by the community

Todo is added. Thanks Manuel Garcia for getting this done! Back to RTBC.

Gábor Hojtsy’s picture

+++ b/core/themes/seven/seven.theme
@@ -186,3 +186,14 @@ function seven_form_node_form_alter(&$form, FormStateInterface $form_state) {
+/**
+ * Implements hook_form_BASE_FORM_ID_alter() for \Drupal\media\MediaForm.
+ */
+function seven_form_media_form_alter(&$form, FormStateInterface $form_state) {

While this looks odd, as discussed above:

1. The same is done in Seven for the node form.
2. Seven is marked internal and therefore allowed to be adjusted like this.

Gábor Hojtsy’s picture

Status: Reviewed & tested by the community » Fixed

I also like the diffstat of the file :) 11 files changed, 66 insertions(+), 90 deletions(-) yay for removing stuff and simplifying the form :)

Thanks all!

  • Gábor Hojtsy committed b45a101 on 8.4.x
    Issue #2863431 by Manuel Garcia, seanB, naveenvalecha, Gábor Hojtsy,...
Manuel Garcia’s picture

Yay! Thanks all for the kind collaboration, patience, explanations and support!

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

idebr’s picture

+++ b/core/themes/seven/seven.libraries.yml
@@ -119,3 +119,11 @@ tour-styling:
+
+media-form:
+  version: VERSION
+  css:
+    layout:
+      css/components/media.css: {}
+  dependencies:
+    - media/form

The dependency on media/form introduces a notice for sites still running the contrib Media Entity module. This is being fixed in #2921529: Seven's media-form library has dependency on Media, which may disabled