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:
Comment | File | Size | Author |
---|---|---|---|
#44 | 2863431-44.patch | 13.56 KB | Manuel Garcia |
#44 | interdiff.txt | 776 bytes | Manuel Garcia |
#42 | Edit File reyery erty try | drupal 8.4.x 2017-07-10 16-30-29.png | 45.41 KB | Gábor Hojtsy |
#41 | Edit File my file | drupal 8.4.x 2017-07-10 16-05-54.png | 32.77 KB | Gábor Hojtsy |
#37 | Screenshot from 2017-07-10 10-15-41.png | 18.36 KB | Manuel Garcia |
Comments
Comment #2
xjmMarking postponed on #2068063: Change "Save and keep un-/published" buttons to a "Published" checkbox and an included "Save" button.
Comment #3
BerdirNote 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 :)
Comment #4
naveenvalechaMain media patch is in #2831274: Bring Media entity module to core as Media module
//Naveen
Comment #5
seanBFirst attempt.
Comment #7
seanBYes, the status checkbox was obviously named differently.
Comment #8
Berdirthe 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.
Comment #9
Manuel Garcia CreditAttribution: Manuel Garcia as a volunteer and at Appnovation commentedRemoving 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.
Comment #10
BerdirLooks 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.
Comment #11
seanBSince the file and image plugins are committed had to replace some more. New patch attached.
Comment #12
seanBForgot the interdiff. Not that exciting, but still.
Comment #14
Manuel Garcia CreditAttribution: Manuel Garcia as a volunteer and at Appnovation commentedComment #15
seanBForgot to add the status field to the image/file media types.
Comment #16
naveenvalechaNice. Patch looks good to me.
Also done the Manual testing and it looks good on widescreen and small screen.
Wide screen
Small Screen
//Naveen
Comment #17
tstoecklerLooking pretty sweet. Have to kick this back though, but I think we're close.
Seems that's a bit high, if it's below the Save button (per the screenshots in #16 (thanks @naveenvalecha!)), no?
The function itself should be removed as well.
Comment #18
Manuel Garcia CreditAttribution: Manuel Garcia as a volunteer and at Appnovation commentedRe #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:
Small screens:
Comment #19
tstoecklerI think you forgot to add the CSS file to the patch. Other than that looks great, and great catch regarding the layout issue!
Comment #20
naveenvalecha#17.2 Nice catch.
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.
Comment #21
Manuel Garcia CreditAttribution: Manuel Garcia as a volunteer and at Appnovation commentedRe #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
Comment #23
naveenvalechaVerified 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
Media add small screen
//Naveen
Comment #24
Gábor HojtsyThe 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?
Comment #25
Manuel Garcia CreditAttribution: Manuel Garcia as a volunteer and at Appnovation commentedRe: #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
andmedia-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.Comment #26
tacituseu CreditAttribution: tacituseu commented@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.
Comment #27
Manuel Garcia CreditAttribution: Manuel Garcia as a volunteer and at Appnovation commentedThanks @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 examplenode.css
: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 =)Comment #28
Gábor HojtsyGiven #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.
Comment #29
timmillwoodIn 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 EntityPublishedInterfaceComment #30
phenaproximaRe-tagging.
Comment #31
marcoscanoUpdating the status to address the border requested in #28, and also:
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.
Comment #32
Manuel Garcia CreditAttribution: Manuel Garcia as a volunteer and at Appnovation commentedRe #29: actually the border is applied to
.layout-region-node-footer__content
, which is a css class living oncore/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
Comment #33
Manuel Garcia CreditAttribution: Manuel Garcia as a volunteer and at Appnovation commentedReroll of #27, there were conflicts on:
Comment #34
Wim LeersI really like the diffstat: 62 insertions, 90 deletions!
I don't know the mechanics of this well enough to provide a helpful review unfortunately.
Comment #35
Manuel Garcia CreditAttribution: Manuel Garcia as a volunteer and at Appnovation commentedUpdating the seven library dependency as #2887269: Remove unnecessary prefixing from media libraries got in.
Comment #36
phenaproximaWelp, looks pretty good to me!
Comment #37
Manuel Garcia CreditAttribution: Manuel Garcia as a volunteer and at Appnovation commented@marcoscano good catch on the weight issue, you were spot on. Addressing that (#31) on this patch.
I feel its ok to leave as RTBC, being this such a small change =)
Comment #38
Manuel Garcia CreditAttribution: Manuel Garcia as a volunteer and at Appnovation commentedComment #39
timmillwoodI'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?
Comment #40
Gábor HojtsyLet'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? :)
Comment #41
Gábor HojtsyTim 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).
Comment #42
Gábor HojtsyJust 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.
Comment #43
Manuel Garcia CreditAttribution: Manuel Garcia as a volunteer and at Appnovation commentedThanks 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.
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?
Comment #44
Manuel Garcia CreditAttribution: Manuel Garcia as a volunteer and at Appnovation commentedOK 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.
Comment #45
seanBTodo is added. Thanks Manuel Garcia for getting this done! Back to RTBC.
Comment #46
Gábor HojtsyWhile 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.
Comment #47
Gábor HojtsyI also like the diffstat of the file :) 11 files changed, 66 insertions(+), 90 deletions(-) yay for removing stuff and simplifying the form :)
Thanks all!
Comment #49
Manuel Garcia CreditAttribution: Manuel Garcia as a volunteer and at Appnovation commentedYay! Thanks all for the kind collaboration, patience, explanations and support!
Comment #51
idebr CreditAttribution: idebr at ezCompany commentedThe 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