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

13715: Added edit button to the ContentPickerField Edit view #13764

Merged

Conversation

dannyd89
Copy link
Contributor

@dannyd89 dannyd89 commented May 29, 2023

Fixes #13715

Added an edit/view button to the ContentPickerField.Edit view which allows to open the selected content item in a separate tab.

Example:
image

Dont know if this is enough or if it's wanted to control the visibility of the button via the field settings?

First time working with vue so feedback is more than welcome, especially on the part with replacing the the content item id into the edit url.

@dannyd89 dannyd89 changed the title 13715: First version added edit button 13715: Added edit button to the ContentPickerField Edit view May 29, 2023
Copy link
Member

@hishamco hishamco left a comment

Choose a reason for hiding this comment

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

Animated GIF it would be helpful to show the final result

@dannyd89
Copy link
Contributor Author

Animated GIF it would be helpful to show the final result

Added an example image with multiple items

@hishamco
Copy link
Member

hishamco commented May 29, 2023

Added an example image with multiple items

So, you didn't open a new window, right?

Added edit button to the ContentPickerField Edit view

I didn't see the edit button on the screenshot!!

@@ -19,16 +20,16 @@
<div class="@Orchard.GetWrapperCssClasses("field-wrapper", $"field-wrapper-{uniqueName.HtmlClassify()}")" id="@Html.IdFor(x => x.ContentItemIds)_FieldWrapper">
<label asp-for="ContentItemIds" class="@Orchard.GetLabelCssClasses()">@Model.PartFieldDefinition.DisplayName()</label>
<div class="@Orchard.GetEndCssClasses()">
<div id="@vueElementId" class="vue-multiselect" data-part="@partName" data-field="@fieldName" data-editor-type="ContentPicker" data-selected-items="@selectedItems" data-search-url="@searchUrl" data-multiple="@multiple">
<div id="@vueElementId" class="vue-multiselect" data-part="@partName" data-field="@fieldName" data-editor-type="ContentPicker" data-selected-items="@selectedItems" data-edit-url="@editUrl" data-search-url="@searchUrl" data-multiple="@multiple">
Copy link
Member

Choose a reason for hiding this comment

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

Why data-edit not data-view or data-display?

Also, you'll need to add a permission check to check if the user can view contentitem before displaying a button that could return 401.

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 wasn't sure about the naming of that, but since its calling the Edit endpoint I went that way. I would be fine with data-view-url if that fits better

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For the permission I would need some guidance on how to do this the proper way, because I cant wrap the button with something like

@if (await AuthorizationService.AuthorizeAsync(User, CommonPermissions.EditContent, contentItem)
 {
       <button v-on:click="edit(item)" type="button" class="btn btn-success content-picker-default__list-item__edit"><i class="fa-solid fa-eye fa-sm" aria-hidden="true"></i></button>
}

Because vue is adding the element to the DOM. Any suggestions how to accomplish the check there?

Copy link
Member

Choose a reason for hiding this comment

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

Update the VueMultiselectItemViewModel by adding a property called IsViewable and do the permission check in the ContentPickerFieldDisplayDriver driver then you would populate the URL if the IsViewable is true. Also, you would have to do similar check when the user selects a new item using the UI.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added checks for both possibilities, button doesn't show up now if the user can't Edit the type

Copy link
Member

Choose a reason for hiding this comment

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

It should be can't View the item.

@dannyd89
Copy link
Contributor Author

dannyd89 commented Jun 2, 2023

Updated looks.

With permission:
image

Without permission:
image

@jtkech
Copy link
Member

jtkech commented Jun 2, 2023

Just an idea on the fly ;)

We needed this in a project where we provided a linkable displayText targeting the editor in place of the additional button, then we had to manage to return back to the parent on save/publish/cancel.

To return back to the parent we had to take into account the use case where the CPField is inside a bagPart, as I remember we had to convert the full uri of the "referrer" header to an absolute url.

@MikeAlhayek
Copy link
Member

@jtkech that is better to me that adding Edit button. You can check if the user can edit, render the display text as a link to the edit page using returnurl as current url. If the user does not have edit, but has view access, show a link to the display page otherwise just display text.

@dannyd89
Copy link
Contributor Author

dannyd89 commented Jun 2, 2023

@jtkech @MikeAlhayek I can take a look into going that route, but Im off for some weeks. I will work on this afterwards again. Thanks for your feedbacks

@sebastienros
Copy link
Member

Is the text displayed in the list of items configurable?

image

If it's the same as the current field then it's ok. Otherwise maybe we should use a custom liquid template

@dannyd89
Copy link
Contributor Author

dannyd89 commented Jul 7, 2023

Is the text displayed in the list of items configurable?

image

If it's the same as the current field then it's ok. Otherwise maybe we should use a custom liquid template

I didn't change anything with how the DisplayText is rendered of an item.
My type just didn't have a TitlePart set, so it's the default ToString of an ContentItem.

Im back now and I hope I can continue working on this in the next days

@dannyd89
Copy link
Contributor Author

dannyd89 commented Aug 13, 2023

Am I right that the View permission is actually implicitly inherited by every role and thus the check if an item is viewable is unnecessary? Do you know that @jtkech or @MikeAlhayek ? Also @jtkech do you think the returnUrl is something we want to have for this one?

@MikeAlhayek
Copy link
Member

MikeAlhayek commented Aug 14, 2023

@dannyd89 the view permission is inherited by default. However, you can change that behavior by removing "View all contents" from the Anonymous role. So the view check is necessary.

The returnUrl will be important to make the user experience better. If the user is sent to the edit view and they published, you want them to be returned to the content item you were on before the navigation away.

IMO, we should have a way to add/edit a related content item using a modal "on the same screen" as it'll have better user experiences. This idea is logged part of issue #11648.

@dannyd89
Copy link
Contributor Author

I think Im finished with the implementation of everything which was requested.

Here you can see an example of a type with a Bag and current logged in user can only view the first 2 items and the other item is restricted to him, so it's not even viewable.
image
The Display urls dont have any returnUrl attached, because you cant do anything on those pages, so I didn't add any returnUrl parameter to it.

With a user which has the permission to edit those items, the UI does look like this:
image
As you can see, the returnUrl is added as a query param and if the user publishes or cancels on the Edit page, he will return to the item with the ContentPickerField he clicked on. The returnUrl contains everything currently in the url string, so even if there is another returnUrl param, it will be captured, so you can click on one item in the ContentPickerField, return to that and if you publish again, you will be returned to whereever you were before.

I hope this contains everything which was requested.

Review is very much appreciated @MikeAlhayek @jtkech

Copy link
Contributor

This pull request has merge conflicts. Please resolve those before requesting a review.

@dannyd89
Copy link
Contributor Author

I sincerely apologize for us taking so much time here. I checked out the PR, going over old ones, and this would be quite useful.

@dannyd89 could you please fix the merge conflict, and @MikeAlhayek please get back to the review?

Resolved all conflicts.
Would be great if this PR is merged soon

@Piedone
Copy link
Member

Piedone commented Mar 22, 2024

Awesome, thank you! @MikeAlhayek could you please finish the review here?

@Skrypt
Copy link
Contributor

Skrypt commented Mar 22, 2024

Maybe I'm late and it is already fixed but why do we get a "view icon" when the action is "edit"? Should we not have an edit icon then? Because, that's what it is if it opens up the edit page of that content item.

@dannyd89
Copy link
Contributor Author

Maybe I'm late and it is already fixed but why do we get a "view icon" when the action is "edit"? Should we not have an edit icon then? Because, that's what it is if it opens up the edit page of that content item.

It's not a button anymore, you can see how it looks with the latest screenshots I posted in the comment above

@Skrypt
Copy link
Contributor

Skrypt commented Mar 22, 2024

ah so it is a anchor

@MikeAlhayek
Copy link
Member

@dannyd89 when the user click the edit or display link, we should open the new link in a new tab by default. If you are in the middle of editing a form and click on a link, you do not want to lose the data you had in the current form.

@dannyd89
Copy link
Contributor Author

@dannyd89 when the user click the edit or display link, we should open the new link in a new tab by default. If you are in the middle of editing a form and click on a link, you do not want to lose the data you had in the current form.

But then the returnUrl parameter doesnt make much sense, does it? I mean I changed it to this way because it was requested, should I change it back?

@MikeAlhayek
Copy link
Member

Hum! The returnUrl maybe helpful if we are just viewing a content, then we click on a user, save changes related to the user and want to get back to the content edit page. I don't know that is a helpful in terms of on UI.

If you are creating a new content, and filled the form half way, then in the user picker you clicked on a user. Now you navigated away, even if you save the user and return back using the returnUrl, you already lost the data you had in the form. And if I end up not saving the user, I am lost and will have to either go back using browser or add new content item.

I think it is better to open another tab if the user click on a user. This way they don't lose any work they have done on current form.

@dannyd89
Copy link
Contributor Author

Hum! The returnUrl maybe helpful if we are just viewing a content, then we click on a user, save changes related to the user and want to get back to the content edit page. I don't know that is a helpful in terms of on UI.

If you are creating a new content, and filled the form half way, then in the user picker you clicked on a user. Now you navigated away, even if you save the user and return back using the returnUrl, you already lost the data you had in the form. And if I end up not saving the user, I am lost and will have to either go back using browser or add new content item.

I think it is better to open another tab if the user click on a user. This way they don't lose any work they have done on current form.

Understood, I changed it so each click will open in a new tab

Copy link
Member

@MikeAlhayek MikeAlhayek left a comment

Choose a reason for hiding this comment

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

LGTM

@Piedone
Copy link
Member

Piedone commented Apr 10, 2024

Can't this be merged now, Mike?

Copy link
Contributor

This pull request has merge conflicts. Please resolve those before requesting a review.

@Piedone
Copy link
Member

Piedone commented Apr 19, 2024

@MikeAlhayek, can't this be now merged? We also need to resolve the merge conflict unless @dannyd89 you can do that (you just need to rebuild the client-side assets, see https://docs.orchardcore.net/en/latest/docs/guides/gulp-pipeline/).

@MikeAlhayek
Copy link
Member

I approved it. If you approve, I say we merge it

…on-contentpickerfield

# Conflicts:
#	src/OrchardCore.Modules/OrchardCore.Resources/wwwroot/Scripts/trumbowyg-plugins.js
@Piedone
Copy link
Member

Piedone commented Apr 19, 2024

Ah OK, just because I was wondering why this was left unmerged after your approval. If you're OK with it them I'm OK as well, I don't think much else is needed here. I merged, could you please do a last check and merge if OK?

@MikeAlhayek MikeAlhayek merged commit 7a6f8da into OrchardCMS:main Apr 19, 2024
5 checks passed
@dannyd89
Copy link
Contributor Author

Thanks @MikeAlhayek and @Piedone for merging it

@Piedone
Copy link
Member

Piedone commented Apr 19, 2024

Thank you for your contribution!

@MikeAlhayek
Copy link
Member

@dannyd89 thanks for your patient as this one took lots of time! and Thank for your contribution. What would your next PR look like?

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

Successfully merging this pull request may close these issues.

Add possibility to open a selected content item in a ContentPickerField in a new tab
7 participants