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

Unified element editor #10467

Merged
merged 10 commits into from Feb 4, 2022
Merged

Conversation

brandonkelly
Copy link
Member

@brandonkelly brandonkelly commented Feb 3, 2022

Description

At a high level, this PR:

  • Introduces a new unified element editing experience, making it much easier for element types to add support for drafts/autosaves, previewing, on-the-fly conditional field layout elements/tabs, and revisions.
  • Brings unpublished + provisional draft and autosave support to element editor slideouts, for element types that support it.
  • Adds draft/autosave/etc. support to categories.
  • Modifies entry/category/asset permissions a bit (see below).

There are some aesthetic improvements as well. Notably, the provisional draft notice + “Discard” button have been moved into the pane header above the edit form, so they’ll be much harder to miss:

Screenshot of the top of an entry form, with a message saying that you are viewing your unsaved changes, and a button to discard them.

Here’s a look at the new slideout editor in action:

CleanShot.2022-02-03.at.10.37.15.mp4

What this means for element types

At a minimum, all element types can take advantage of the new element editor via element slideouts, which are now powered by it. They can also choose to point their full edit pages over to it (as entries, categories, and assets are now doing) by changing their edit page route to point to elements/edit with an elementId param.

To take full advantage, element types should start extending the following methods, where appropriate:

  • canView() – Whether the given user is authorized to view the element.
  • canSave() – Whether the given user is authorized to save the element.
  • canDuplicate() – Whether the given user is authorized to duplicate the element.
  • canDelete() – Whether the given user is authorized to delete the element.
  • canDeleteForSite() – Whether the given user is authorized to delete the element for its currently-loaded site only. (Requires that the element type supports a manual site propagation mode like entries have when their section is set to “Let each entry choose which sites it should be saved to”.)
  • canCreateDrafts() – Whether the given user is authorized to create drafts for the element.
  • createAnother() – Returns a new, unsaved element for “Save and add another” actions. (No need to worry about permissions here; canSave() will be in effect for it.)
  • hasRevisions() – Whether the element type is creating revisions for itself.
  • getPostEditUrl() – The URL that the browser should be redirected to after the element is saved.
  • getCrumbs() – Array of breadcrumbs that should be shown on the element’s edit page.
  • getAddlButtons() – HTML for any additional buttons that should be shown beside “Save”.

getEditorHtml() has been removed. The main editor content area is now solely determined by the element’s rendered field layout form.

Elements’ getSidebarHtml(), metaFieldsHtml(), and metadata() methods continue to be used by the editor sidebar (both for slideouts and full edit pages now). An additional security measure has been put in place though: any posted values will be set on the element via its setAttributes() method, which by default will only actually allow “safe” attributes (attributes which have corresponding validation rules). So element types’ defineRules() methods will need to fully cover any inputs that could be posted via getSidebarHtml()/metaFieldsHtml().

More to be aware of

The following have been removed:

  • The entries/edit-entry, categories/edit-category, and assets/edit-asset controller actions and corresponding templates.
  • The _layouts/element template.
  • The Craft.BaseElementEditor JS class.

The entries/save-entry, categories/save-category, and assets/save-assets controller actions are still around, but deprecated. We may remove it in v5+, so moving to elements/save is encouraged.

Entry, category, and asset permissions have also been cleaned up a bit:

Entries:

  • editEntries:<uid>viewEntries:<uid>
  • publishEntries:<uid>saveEntries:<uid>
  • editPeerEntryDrafts:<uid>viewPeerEntryDrafts:<uid> and savePeerEntryDrafts:<uid>
  • editPeerEntries:<uid>viewPeerEntries:<uid>
  • publishPeerEntries:<uid>savePeerEntries:<uid>

(There’s no longer a distinction between saving an enabled or disabled entry, which is what “publish” used to refer to here. Nowadays if a user wants to make a non-live change to an element, they can create a draft.)

Categories:

  • editCategories:<uid>viewCategories:<uid>, saveCategories:<uid>, deleteCategories:<uid>, viewPeerCategoryDrafts:<uid>, savePeerCategoryDrafts:<uid>, and deletePeerCategoryDrafts:<uid>

Assets:

  • viewVolume:<uid>viewAssets:<uid>
  • saveAssetInVolume:<uid>saveAssets:<uid>
  • deleteFilesAndFoldersInVolume:<uid>deleteAssets:<uid>
  • replaceFilesInVolume:<uid>replaceFiles:<uid>
  • editImagesInVolume:<uid>editImages:<uid>
  • viewPeerFilesInVolume:<uid>viewPeerAssets:<uid>
  • editPeerFilesInVolume:<uid>savePeerAssets:<uid>
  • replacePeerFilesInVolume:<uid>replacePeerFiles:<uid>
  • deletePeerFilesInVolume:<uid>deletePeerAssets:<uid>
  • editPeerImagesInVolume:<uid>editPeerImages:<uid>
  • createFoldersInVolume:<uid>createFolders:<uid>

All existing users and user groups will be automatically updated to use the new permissions.

Related issues

@linear
Copy link

linear bot commented Feb 3, 2022

DEV-230 Unified element editor

Combine element edit pages and slideouts into a unified element editor screen.

Should extend draft/revision support to all element types (that want it) as well.

@brandonkelly brandonkelly merged commit dfb2048 into 4.0 Feb 4, 2022
@brandonkelly brandonkelly deleted the feature/dev-230-unified-element-editor branch February 4, 2022 01:35
@BenParizek
Copy link
Contributor

@brandonkelly Neat stuff. On my first pass this is simplifying a lot of things for managing how elements are edited.

Can I ask you to clarify what assumptions we should have when dealing with New Elements? Most of the actionCreate methods I'm looking at seem to be using saveElementAsDraft. Would it now be a best practice to have any new Element create a temporary Draft when a user is creating it for the first time? Does this mean we are opting in to Draft's in any greater sense or can we just lean on Drafts when creating a new Element? And if a user abandons a new Draft Element, will the Draft clean itself up or is that something we have to manage? It looks like any required fields are just excluded in the rules via 'except' => [self::SCENARIO_ESSENTIALS],?

Sorry for the pile of questions. I'm mostly just trying to update my previous mental models on how all this worked and get pointed in the right direction. If I'm jumping the gun and this will all get documented with the tagged release, just let me know.

@BenParizek
Copy link
Contributor

BenParizek commented Feb 26, 2022

Ok, I've confirmed custom elements do not need to support Drafts, as you likely know =). My main issue was just making sure required fields are not required on the first save when the element is created and still disabled.

While I'm working through my issues one by one and overall liking these updates, one more issue I'm noticing is that Element::displayName() is always used for the first column of the Table now (in the ElementSources::getTableAttributes method) and there doesn't appear to be a way to override it. In the past, I was able to rename that first column by returning something different in the _toString method. Now, Even though I can return something different than title in the toString method, the column in the Element Index results is always titled the name of the Element.

In my case, I have a Redirect Element that does not have titles. The first row is more appropriately titled "Old Url" and is next to the second column, "New Url". In Craft 4, the first column is now called "Redirect" and I can't seem to get around it unless I rename the custom Element "Old Url".

@brandonkelly
Copy link
Member Author

elements/create will generate an “unsaved,” unpublished draft – it’s technically a draft, but it’s also the canonical (and only) version of the element. We do this because it gives you a way to start working on an element that already has an ID, but if it never ends up getting any follow-up saves, it will be garbage collected eventually. You can follow suit without actually supporting derivative drafts if you want. The editor will automatically drop its draft status the first time its “Create” button is pressed.

While I'm working through my issues one by one and overall liking these updates, one more issue I'm noticing is that Element::displayName() is always used for the first column of the Table now (in the ElementSources::getTableAttributes method) and there doesn't appear to be a way to override it. In the past, I was able to rename that first column by returning something different in the _toString method. Now, Even though I can return something different than title in the toString method, the column in the Element Index results is always titled the name of the Element.

This shouldn’t have changed in v4. Are you overriding uiLabel() by chance?

@BenParizek
Copy link
Contributor

@brandonkelly Looking at this a bit more closely, uiLabel seems to affect the Element names themselves. What I'm after is a way to modify the Element Index Header name, which uses the displayName method. The issue seems to be this line of code that is assuming the Element displayName is the most meaningful label for the first column:

$attributes = [
// Start with the element type’s display name
['title', ['label' => $elementType::displayName()]],
];

If I change that to "Old Url" for the Element Index page, it also changes the Edit page to say "Edit Old Url" instead of "Edit Redirect". So, I think I'm looking for a way to override the displayName method just on the Element Index page, or some other way to define the elementIndex display name for the first column header.

The element's indexHtml method does have a viewState which signals it's a table but I'm unable to override that heading here either without copying the entire method just to tweak that one word.

cms/src/base/Element.php

Lines 975 to 992 in 42def75

if ($viewState['mode'] === 'table') {
// Get the table columns
$variables['attributes'] = Craft::$app->getElementSources()->getTableAttributes(static::class, $sourceKey);
// Give each attribute a chance to modify the criteria
foreach ($variables['attributes'] as $attribute) {
$event = new ElementIndexTableAttributeEvent([
'query' => $elementQuery,
'attribute' => $attribute[0],
]);
Event::trigger(static::class, self::EVENT_PREP_QUERY_FOR_TABLE_ATTRIBUTE, $event);
if (!$event->handled) {
static::prepElementQueryForTableAttribute($elementQuery, $attribute[0]);
}
}
}

The lightest touch way I've found to override this for now is to insert my own, temporary session variable in the Element's indexHtml method:

public static function indexHtml(ElementQueryInterface $elementQuery, ?array $disabledElementIds, array $viewState, ?string $sourceKey, ?string $context, bool $includeContainer, bool $showCheckboxes): string
    {
        Craft::$app->getSession()->set('sprout-redirect-element-index-display-name', 'Old Url');

        return parent::indexHtml($elementQuery, $disabledElementIds, $viewState, $sourceKey, $context, $includeContainer, $showCheckboxes);
    }

And retrieve it in my displayName method.

public static function displayName(): string
    {
        if ($displayName = Craft::$app->getSession()->get('sprout-redirect-element-index-display-name')) {
            Craft::$app->getSession()->remove('sprout-redirect-element-index-display-name');

            return $displayName;
        }

        return Craft::t('sprout-module-redirects', 'Redirect');
    }

Not very elegant but gets me the naming conventions I'd like to have on the Element Index and Element Edit pages.

I'm not sure if any other Elements have the UX scenario of not having the concept of a title and wanting to name that first column differently than the Element Name itself, but for my situation with Redirects it'd be nice if displayName was aware of context displayName($viewMode) or there was some way I could override the heading label of that title column header on the index page without having to duplicate the entire indexHtml method.

@brandonkelly
Copy link
Member Author

What I'm after is a way to modify the Element Index Header name

Ah gotcha… support for customizing the header column heading has been intentionally removed, for accessibility reasons. It’s confusing (especially to screen readers) when the column references a particular attribute of the elements (e.g. their title) yet the actual table cells contain additional information, such as their status, thumbnail, etc.

@BenParizek
Copy link
Contributor

BenParizek commented Mar 7, 2022

My Redirect Element does not have a Title. What would you vote is the better experience for a screen reader below? From a UI/Naming perspective, I find both options unnecessarily confusing.

Before

  • Column 1: "Old URL" => The Old Url being redirected
  • Column 2: "New URL" => The New Url to redirect to

Now

Option 1:

  • Column 1: "Redirect" => The Old Url being redirected
  • Column 2: "New URL" => The New Url to redirect to

Option 2:

  • Column 1: "Redirect" => The Old Url being redirected
  • Column 2: "Old URL" => The Old Url being redirected
  • Column 2: "New URL" => The New Url to redirect to

@brandonkelly
Copy link
Member Author

Hm, awkward. I’m not sure it makes any sense that Old URL was the thing getting the link previously either though. Maybe you should just output the redirect IDs as the “display name”? So:

  • Column 1: “Redirect” → the ID
  • Column 2: “Old URL”
  • Column 3: “New URL”

@BenParizek
Copy link
Contributor

I agree there's not a perfect candidate to link to here but I'll probably try my silly session hack as I feel the Old/New distinction is the most clear to the majority of users as to what the actual data is. The ID is irrelevant to the use case, and might be hard to click on if it's just two or three digits (and I would imagine just as irrelevant to a screen reader? I'm not sure what's being read there as the concept of "title" is irrelevant here too so it really won't make sense to anybody until they get to column 2 and 3). And the uris can potentially be lengthy so trying to combine Old/New into the Title field as a link will likely lead to some long, wrapping title scenarios that may make the data less clear.

So, the non-title Element Visual version:

  • Column 1: “Old URL”
  • Column 2: “New URL”

And the non-title Element screen-reader version

  • Column 1: “Title” (which reads out Old URL and actually matters to the use case)
  • Column 2: “New URL”

Anyway, not that you care too much about my specific use case. Just to share how some random guy on the internet is using your APIs =)

@brandonkelly
Copy link
Member Author

What if the label was actually both URLs? E.g. <old URL> → <new URL>

@yoannisj
Copy link

@brandonkelly , really excited to see the experience and feature-set around editing of Element types being consolidated and more uniform. Thanks also for the clear and detailed walk-through of the changes in this PR's description :).

Something tickles me a bit however:

(There’s no longer a distinction between saving an enabled or disabled entry, which is what “publish” used to refer to here. Nowadays if a user wants to make a non-live change to an element, they can create a draft.)

Does this mean that –with the new built-in permissions– it is not possible anymore to prevent some users from bringing their edits live (i.e. they are restricted to work only within drafts)?

This is a use-case on some of our projects, and I believe the excellent Workflow plugin heavily builds upon this.


If the built-in permissions don't allow this anymore, I think it could be re-introduced by registering a custom permission and listening to element saves to check the element's enabled/disabled status (and/or whether it is a draft or not if the element type supports drafts and revisions). Would that be the recommended way to restrict users from bringing their changes live?

@brandonkelly
Copy link
Member Author

Does this mean that –with the new built-in permissions– it is not possible anymore to prevent some users from bringing their edits live (i.e. they are restricted to work only within drafts)?

No. Users who were previously unable to “publish” entries (save them as live) will be limited to only saving entries as drafts in Craft 4, and leave it up to another user with save permissions to actually publish the draft.

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.

None yet

3 participants