Skip to content

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

Merged
brandonkelly merged 62 commits into5.3from
feature/cms-1091-ability-to-move-entries-to-a-new-section
Jul 24, 2024
Merged

Feature/cms 1091 ability to move entries to a new section#14541
brandonkelly merged 62 commits into5.3from
feature/cms-1091-ability-to-move-entries-to-a-new-section

Conversation

@i-just
Copy link
Copy Markdown
Contributor

@i-just i-just commented Mar 6, 2024 •
edited
Loading

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

@linear
Copy link
Copy Markdown

linear Bot commented Mar 6, 2024

CMS-1091 Ability to move entries to a new section

i-just and others added 10 commits March 8, 2024 10:16
@i-just
Merge branch '5.0' into feature/cms-1091-ability-to-move-entries-to-a…
ce8d275 
…-new-section
@i-just
allow move when too many authors
dd8040e
@i-just
move drafts & revisions too
1192188
@i-just
safeguard applying drafts & reverting to revisions
e2a2420
@brianjhanson
Update tooltip to work on disabled elements
b527083
@i-just
Merge branch '5.0' into feature/cms-1091-ability-to-move-entries-to-a…
f650ae1 
…-new-section
@i-just
Merge pull request #14561 from craftcms/feature/tooltip-on-diabled-el…
78f2e7a 
…ements

Update tooltip to work on disabled elements
@i-just
Merge branch 'feature/cms-1091-ability-to-move-entries-to-a-new-secti…
0f40cfa 
…on' of https://github.com/craftcms/cms into feature/cms-1091-ability-to-move-entries-to-a-new-section
@i-just
tooltip for disabled revert to revision
2d3a431
@i-just
compiled assets
4a50089
@i-just i-just marked this pull request as ready for review March 11, 2024 10:15
@brandonkelly
Merge branch '5.0' into feature/cms-1091-ability-to-move-entries-to-a…
7fb8f0e 
…-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
Copy Markdown
Member

brandonkelly commented Mar 11, 2024

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.

gcamacho079
gcamacho079 requested changes Mar 12, 2024
View reviewed changes
Copy link
Copy Markdown
Contributor

@gcamacho079 gcamacho079 left a comment •
edited by i-just
Loading

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.

@brandonkelly brandonkelly changed the base branch from 5.0 to 5.1 March 19, 2024 16:31
brandonkelly
brandonkelly reviewed Mar 19, 2024
View reviewed changes
Comment thread src/controllers/ElementsController.php Outdated
brandonkelly
brandonkelly reviewed Mar 19, 2024
View reviewed changes
Comment thread src/base/ElementInterface.php Outdated
i-just and others added 8 commits March 20, 2024 19:55
@i-just
different way of showing incompatible entry type tooltip
f86e782
@i-just
keep moving within Entry (not Element)
4a301fa
@i-just
ability to add additional chip/card attrs from element type
62177ee
@brandonkelly
Merge branch '5.x' into feature/cms-1091-ability-to-move-entries-to-a…
0c760ac 
…-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
@brandonkelly
Merge branch '5.1' into feature/cms-1091-ability-to-move-entries-to-a…
f8bda27 
…-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
Set data-movable via htmlAttributes()
70bcc97
@brandonkelly
Merge branch '5.1' into feature/cms-1091-ability-to-move-entries-to-a…
a1b1d82 
…-new-section
@brandonkelly
Cleanup
69be479 
- Made Entry::canMove() private
- Moved moveEntryToSection() to the Entries service
@brandonkelly brandonkelly changed the base branch from 5.1 to 5.2 April 30, 2024 15:18
@brandonkelly brandonkelly changed the base branch from 5.2 to 5.3 June 12, 2024 16:35
@gcamacho079
Copy link
Copy Markdown
Contributor

gcamacho079 commented Jul 23, 2024 •
edited by brandonkelly
Loading

A few accessibility improvements based on automated and manual testing:

  • The checkbox role doesn’t communicate the correct interaction pattern. Radio inputs would work better since the selections are exclusive.
    • Radio inputs should be contained in a fieldset with a legend/aria-labelledby that references the title of the modal
    • List markup should be removed as radio group markup should provide the necessary relationship information
  • The accessible name of the inputs should match the visible name (i.e., the section name). Right now, the label ID that’s referenced via aria-labelledby does not exist, so the name is coming from the input title.
  • Focus is dropped when an entry is moved. In these instances, focus should probably be moved to the .elements container. Ensure programmatic focus can be set by adding `tabindex="-1" to the container.

@brandonkelly brandonkelly merged commit e75a51a into 5.3 Jul 24, 2024
@brandonkelly brandonkelly deleted the feature/cms-1091-ability-to-move-entries-to-a-new-section branch July 24, 2024 18:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@brandonkelly brandonkelly brandonkelly left review comments

@gcamacho079 gcamacho079 gcamacho079 approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@i-just @brandonkelly @gcamacho079 @brianjhanson