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

Publish and Save Draft buttons : add "and create new" option #3623

Open
wants to merge 29 commits into
base: dev
from

Conversation

Projects
None yet
2 participants
@Skrypt
Copy link
Contributor

commented May 12, 2019

Add "and create new option" to save time while adding new content items.

image
image

Fixes #3632

@Skrypt Skrypt added the notready label May 12, 2019

@Skrypt

This comment has been minimized.

Copy link
Contributor Author

commented May 12, 2019

@matiasmolleja can you remember me why we we're adding this returnUrl condition in Content.PublishButton.cshtml and Content.SaveDraftButton.cshml?

@if (String.IsNullOrWhiteSpace(returnUrl))
{
    <button type="submit" name="submit.Publish" class="publish-button btn btn-success" value="submit.Publish">@T["Publish"]</button>
}
else
{
    <div class="btn-group">
        <button class="publish-button btn btn-success" type="submit" name="submit.Publish" value="submit.Publish">@T["Publish"]</button>
        <button type="button" class="btn btn-success dropdown-toggle dropdown-toggle-split" data-reference="parent" data-toggle="dropdown" aria-haspopup="true" aria-expanded="false">
            <span class="sr-only">@T["Toggle Dropdown"]</span>
        </button>
        <div class="dropdown-menu">
            <button class="dropdown-item" type="submit" name="submit.Publish" value="submit.PublishAndContinue">@T["and continue"]</button>
        </div>
    </div>
}
@matiasmolleja

This comment has been minimized.

Copy link
Member

commented May 13, 2019

I think it is because when we don't have a return url , we will stay in the same page once we publish.
It would be weird having two buttons ("publish", and "publish and continue") when both will do the same thing: publish and continue.

@Skrypt

This comment has been minimized.

Copy link
Contributor Author

commented May 13, 2019

Then maybe we should just add a returnUrl to the contentItems list in the admin menu create --> items list? I understand now, but I would still like to have the option even when I'm creating a new content item. Let's just fix the issue by adding a returnUrl.

@Skrypt

This comment has been minimized.

Copy link
Contributor Author

commented May 14, 2019

I'm refactoring a third time the way I implementing this. I started trying to add the .ReturnUrl() option to the NavigationItemBuilder. Seems like the natural way to implement this.

@Skrypt Skrypt removed the notready label May 14, 2019

Skrypt added some commits May 15, 2019

@Skrypt

This comment has been minimized.

Copy link
Contributor Author

commented May 15, 2019

Done changes related to comments from tuesday meeting @sebastienros

Skrypt added some commits May 15, 2019

Use hidden input instead for returnUrl.
Reverting changes on NavigationItermBuilder.

Skrypt added some commits May 15, 2019

Revert "Use hidden input instead for returnUrl."
This reverts commit 2d60fda.

# Conflicts:
#	src/OrchardCore.Modules/OrchardCore.Contents/Views/Admin/Create.cshtml
#	src/OrchardCore.Modules/OrchardCore.Contents/Views/Admin/Edit.cshtml
#	src/OrchardCore.Modules/OrchardCore.Contents/Views/Content.PublishButton.cshtml
#	src/OrchardCore.Modules/OrchardCore.Contents/Views/Content.SaveDraftButton.cshtml
#	src/OrchardCore/OrchardCore.Navigation.Core/NavigationManager.cs
@Skrypt

This comment has been minimized.

Copy link
Contributor Author

commented May 15, 2019

Using a hidden input for returnUrl was not appropriate because the context where we add content items could change. This is why for example we do a returnUrl to the Layers editor page when we create a Widget and we do a returnUrl to the content items list when adding a content item from the admin menu. The buttons themselves defines where we are doing the returnUrl because it defines also the context. For that matter I reverted those changes and reinserted the proposed changes in the NavigationItemBuilder.

Skrypt added some commits May 15, 2019

Merge remote-tracking branch 'origin/dev' into skrypt/create-and-add
# Conflicts:
#	src/OrchardCore.Modules/OrchardCore.Contents/Controllers/AdminController.cs

@Skrypt Skrypt added notready and removed ready labels May 20, 2019

@Skrypt

This comment has been minimized.

Copy link
Contributor Author

commented May 21, 2019

I found an issue with the "and create new" when we are in the context of editing widgets.

For widget the params missing are for example : &LayerMetadata.Zone=Footer&LayerMetadata.Position=12

The issue here is that we are passing params when using the create button to define in wich zone and position the Layer should be and when we are editing the created item the url doesn't require having these params so they are never passed.

@Skrypt Skrypt added ready and removed notready labels May 29, 2019

@Skrypt

This comment has been minimized.

Copy link
Contributor Author

commented May 31, 2019

Need to test with menu items creation. Might fail. 😫
Tested and the "and create new" doesn't work with menu items. Need to fix it. But the form post works properly and redirects to the menu page.

@Skrypt Skrypt added notready and removed ready labels Jun 1, 2019

@Skrypt Skrypt added ready and removed notready labels Jun 5, 2019

@Skrypt

This comment has been minimized.

Copy link
Contributor Author

commented Jun 5, 2019

I have covered Layers, Menu and Contents modules with this PR. If there is any other module that uses these common buttons that I missed let me know.

return LocalRedirect(returnUrl);
}
var routeValueDictionnary = new RouteValueDictionary { { "id", id }, { "returnUrl", returnUrl } };
var layerMetadata = contentItem.As<LayerMetadata>();

This comment has been minimized.

Copy link
@Skrypt

Skrypt Jun 5, 2019

Author Contributor

To be honest this would require a second thought. This makes the Contents module dependent on the Layers module which is not so good.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.