Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

#8565: Delete and Unpublish buttons in backoffice content edit #8566

Open
wants to merge 13 commits into
base: 1.10.x
Choose a base branch
from

Conversation

AndreaPiovanelli
Copy link
Contributor

@AndreaPiovanelli AndreaPiovanelli commented Jun 10, 2022

Fixes #8565

Added unpublish and delete buttons to backoffice content edit view.
Adapted Orchard.Blogs and Orchard.Widgets to be in line with the patch, adding a "unpublish-button" css class too (it is identical to "publish-button".
Regarding Orchard.Widgets, I removed previous delete button (Widget_DeleteButton shape, Widget.DeleteButton.cshtml) because it was now redundant and conflicted with the Core/Contents... button (not a terrible conflict, but both were displayed and editing the Placement.info turned to be a unreliable solution).
Modified / added actions to controllers when needed.

@@ -352,6 +352,18 @@ public class AdminController : ContentControllerBase, IUpdateModel {
return EditPOST(id, returnUrl, contentItem => _contentManager.Publish(contentItem));
}

[HttpPost, ActionName("Edit")]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this different that the other unpublish/delete actions?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added delete and unpublish actions referencing specific submits to the controller. There already were unpublish and delete actions (the ones called from the lists), so I called those inside the new actions, specific to the edit form.

@AndreaPiovanelli
Copy link
Contributor Author

@sebastienros fixed as requested.

@AndreaPiovanelli
Copy link
Contributor Author

Modified the code following Sebastien's remarks.

@BenedekFarkas BenedekFarkas linked an issue Apr 16, 2024 that may be closed by this pull request
Copy link
Member

@BenedekFarkas BenedekFarkas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I really like the consolidation for the delete button! Please merge from 1.10.x.

However I'm unsure about adding the Unpublish button in the editor - is that really necessary UX-wise? I feel like it get can get a bit too crowded in the action button row.

Some of the editors I checked have minor styling issues under different circumstances, but some of those might be independent of this PR:

  1. BlogPost, Page, etc., when Draftable is enabled (default configuration) - too much space
    image
  2. BlogPost, when Draftable is disabled - spacing missing:
    image

EDIT: These spacing problems are addressed by my suggestions below.

Comment on lines +26 to +28
if (part.ContentItem.IsPublished()) {
results.Add(ContentShape("Content_UnpublishButton", unpublishButton => unpublishButton));
}
Copy link
Member

@BenedekFarkas BenedekFarkas Apr 16, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re: the previous comment - shouldn't be just this then, since the template already checks IsPublished?

Suggested change
if (part.ContentItem.IsPublished()) {
results.Add(ContentShape("Content_UnpublishButton", unpublishButton => unpublishButton));
}
results.Add(ContentShape("Content_UnpublishButton", unpublishButton => unpublishButton));

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't remember right now. I'll check asap.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The Content.UnpublishButton template does check if the item is published.

@BenedekFarkas BenedekFarkas changed the title Delete and Unpublish buttons in backoffice content edit #8565: Delete and Unpublish buttons in backoffice content edit Apr 18, 2024
@MatteoPiovanelli-Laser
Copy link
Contributor

rebased on 1.10.x and applied some of @BenedekFarkas suggestions

@BenedekFarkas
Copy link
Member

Sorry for replying just now, I'll be able to review later towards the end of September (after Harvest).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please use remove and sort usings and auto-formatting.

@@ -0,0 +1,9 @@
@using Orchard.ContentManagement;
@using Orchard.Core.Contents;
@using Orchard.Utility.Extensions
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unused using.

@@ -0,0 +1,11 @@
@using Orchard.ContentManagement;
@using Orchard.Core.Contents;
@using Orchard.Utility.Extensions;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unused using.

Comment on lines +4 to +6
@{
var contentItem = Model.ContentItem as IContent;
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add empty lines around this block. I pointed this out earlier too and it would be easier if you just accepted my change suggestions in bulk, so that I don't have to go through all of them again to figure out what's fixed and what isn't.

Suggested change
@{
var contentItem = Model.ContentItem as IContent;
}
@{
var contentItem = Model.ContentItem as IContent;
}

Comment on lines +26 to +28
if (part.ContentItem.IsPublished()) {
results.Add(ContentShape("Content_UnpublishButton", unpublishButton => unpublishButton));
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The Content.UnpublishButton template does check if the item is published.

Comment on lines +9 to +12
<Place Content_UnpublishButton="Sidebar:25"/>
<Place Content_PublishButton="Sidebar:24"/>
<Place Content_SaveButton="Sidebar:23"/>
<Place Content_DeleteButton="Sidebar:22"/>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reordered them according to placement. Also Content_DeleteButton shouldn't be the first one, because it will be become the default action (e.g., when you hit enter while editing the title) just by virtue of being the first in the rendered HTML.
SaveButton should come first, then PublishButton (because SaveButton is not rendered if the type is not Draftable).

Suggested change
<Place Content_UnpublishButton="Sidebar:25"/>
<Place Content_PublishButton="Sidebar:24"/>
<Place Content_SaveButton="Sidebar:23"/>
<Place Content_DeleteButton="Sidebar:22"/>
<Place Content_SaveButton="Sidebar:23"/>
<Place Content_PublishButton="Sidebar:24"/>
<Place Content_UnpublishButton="Sidebar:25"/>
<Place Content_DeleteButton="Sidebar:26"/>

clear:none;
float:left;
}
fieldset.save-button {
clear:left;
}
fieldset.publish-button {
fieldset.publish-button, fieldset.unpublish-button {
margin: 0 12px 0 0;
padding: 0 12px;
border-right:1px solid #ccc;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This styling was overcomplicated to begin with, but we don't need to complicate it further. You can apply this to the whole section between lines 1128 and 1147, so we don't need to care about how many items are rendered and in what order:

.edit-item-sidebar fieldset {
    margin: 0;
    padding: 0;
    clear: none;
    float: left;
}
.edit-item-sidebar fieldset:not(:first-child) {
    margin-left: 12px;
}
.edit-item-sidebar fieldset:first-child {
    clear: left;
}
fieldset.publish-button, fieldset.unpublish-button {
    padding-right: 12px;
    border-right: 1px solid #ccc;
}
fieldset.delete-button {
    float: right;
}

@@ -1495,13 +1495,13 @@ html.dir-rtl {
margin-left:inherit;
margin-right:10px;
}
.dir-rtl fieldset.publish-button, fieldset.delete-button, .dir-rtl fieldset.save-button {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as above, RTL version for lines 1498-1509:

.dir-rtl .edit-item-sidebar fieldset {
    float: right;
}
.dir-rtl .edit-item-sidebar fieldset:first-child {
    clear: right;
}
.dir-rtl .edit-item-sidebar fieldset:not(:first-child) {
    margin-left: 0;
    margin-right: 12px;
}
.dir-rtl fieldset.publish-button, fieldset.unpublish-button {
    padding-right: 0;
    border-right: none;
    padding-left: 12px;
    border-left: 1px solid #ccc;
}
.dir-rtl fieldset.delete-button {
    float: left;
}

Comment on lines +39 to +42
if (widgetPart.Id > 0 && widgetPart.TypeDefinition.Settings.GetModel<ContentTypeSettings>().Draftable) {
results.Add(ContentShape("Content_UnpublishButton",
unpublishButton => unpublishButton));
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is already rendered by ContentsDriver.

Suggested change
if (widgetPart.Id > 0 && widgetPart.TypeDefinition.Settings.GetModel<ContentTypeSettings>().Draftable) {
results.Add(ContentShape("Content_UnpublishButton",
unpublishButton => unpublishButton));
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unpublish and Delete buttons in backoffice content edit page
5 participants