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

Feature/cms 1091 ability to move entries to a new section #14541

Open
wants to merge 51 commits into
base: 5.2
Choose a base branch
from

Conversation

i-just
Copy link
Contributor

@i-just i-just commented Mar 6, 2024

Description

Adds element index action to move entries between sections.

  • select entries to be moved and trigger the “Move to” action
Screenshot 2024-03-06 at 12 06 58
  • see a modal with a list of compatible sections (sections that have the same entry type(s) as the entries we want to move)
Screenshot 2024-03-06 at 12 07 10
  • choose a section to move to
  • entries are moved to the new section
Screenshot 2024-03-06 at 12 07 17

Notes:

  • it’s not possible to move to a single section
  • for structures, entries are moved to the end of the new structure as root elements
  • if the section entries are moved to has a lower maxAuthors limit, no data is changed/lost for the entries, but the next time they’re saved, a validation error will be shown
  • when an entry is moved, its revisions and drafts are moved too; if their entry type isn’t allowed by the section we’re moving to, you will still be able to see the draft/revision but not apply/revert to it (the same applies to revisions and draft for entry types that were removed from the section); thanks @brianjhanson for adjusting the awesome craft-tooltip to work with disabled elements!
Screenshot 2024-03-11 at 09 59 42 Screenshot 2024-03-11 at 10 00 13

Related issues

cms-1091

Copy link

linear bot commented Mar 6, 2024

@i-just i-just marked this pull request as ready for review March 11, 2024 10:15
…-new-section

# Conflicts:
#	src/web/assets/cp/dist/cp.js
#	src/web/assets/cp/dist/cp.js.map
#	src/web/assets/cp/dist/css/cp.css
#	src/web/assets/cp/dist/css/cp.css.map
@brandonkelly
Copy link
Member

Rather than showing the “Couldn’t find any sections that all selected elements could be moved to.” message in the modal, ideally the “Move to” action is just grayed out ahead of time if there aren’t any other sections with the same entry type.

This should be doable via the validateSelection() method on the action trigger JS.

Copy link
Contributor

@gcamacho079 gcamacho079 left a comment

Choose a reason for hiding this comment

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

Just finished a quick scan of the modal contents:

  • The section buttons should have an aria-pressed value that toggles between true and false (see internal documentation re: exclusive button groups)
  • The Move button's aria-disabled state should be toggled when a section is selected
  • The focus indicator for the move icon is under 3:1 contrast ratio against the modal footer background. This is a larger global issue caused by the disabled element opacity.
  • The focus indicator for the button is difficult to see since it doesn’t contrast against the dark background. This could benefit from a two-toned focus indicator, like the one used in the global skip link, with a white outline adjacent to the button background.
  • Loading spinner that appears in modal content body should have an equivalent status message that is read for screen reader users.

Note that some of these (like the disabled button focus outline) may be larger/more global issues than the scope of this PR.

@i-just i-just marked this pull request as draft March 14, 2024 15:47
@i-just
Copy link
Contributor Author

i-just commented Mar 15, 2024

@gcamacho079, as per our chats, all items except the disabled button focus ring are actioned.

To get the last item from the list to work, I added the live region container and updateLiveRegion method to all modals (via Garnish.Modal) and started using it in my EntryMover, which means other modals could start taking advantage of it.

@i-just i-just marked this pull request as ready for review March 15, 2024 07:58
@i-just i-just requested a review from gcamacho079 March 15, 2024 08:00
@brandonkelly brandonkelly changed the base branch from 5.0 to 5.1 March 19, 2024 16:31
$components[] = Html::button(Craft::t('app', 'Apply draft'), [
'class' => ['btn', 'secondary', 'formsubmit'],
/** @phpstan-ignore-next-line */
$disabled = $canonical->hasMethod('isEntryTypeCompatible') && !$element->isEntryTypeCompatible();
Copy link
Member

Choose a reason for hiding this comment

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

ElementsController shouldn’t be giving any special treatment to entries, so we’ll need to rethink how we do this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok; addressed here: f86e782

* @return bool
* @since 5.0.0
*/
public function canMove(User $user): bool;
Copy link
Member

Choose a reason for hiding this comment

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

It seems a little weird to introduce the concept of “moving” to all element types, when entries are the only one that would actually know what to do with it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As per our chat, this might end up being a way to go.
But, for now, I have reverted those changes and used a different approach where I added the ability to further customise element chip & card attributes, and I’m adding data-movable via those.

i-just and others added 7 commits March 20, 2024 19:55
…-new-section

# Conflicts:
#	.github/workflows/ci.yml
#	src/elements/Entry.php
#	src/web/assets/cp/dist/cp.js
#	src/web/assets/cp/dist/cp.js.map
#	src/web/assets/cp/dist/css/cp.css
#	src/web/assets/cp/dist/css/cp.css.map
#	src/web/assets/cp/src/js/CraftElementLabel.js
#	src/web/assets/garnish/dist/garnish.js
#	src/web/assets/garnish/dist/garnish.js.map
…-new-section

# Conflicts:
#	src/web/assets/cp/dist/cp.js
#	src/web/assets/cp/dist/cp.js.map
#	src/web/assets/cp/dist/css/cp.css
#	src/web/assets/cp/dist/css/cp.css.map
@brandonkelly brandonkelly changed the base branch from 5.1 to 5.2 April 30, 2024 15:18
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

4 participants