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

🎛️ Migrate Page re-ordering to a Stimulus controller w-orderable #10909

Closed
lb- opened this issue Sep 18, 2023 · 12 comments
Closed

🎛️ Migrate Page re-ordering to a Stimulus controller w-orderable #10909

lb- opened this issue Sep 18, 2023 · 12 comments
Milestone

Comments

@lb-
Copy link
Member

lb- commented Sep 18, 2023

ℹ️ Part of the Stimulus 🎛️ RFC 78

Is your proposal related to a problem?

At the moment we have an ad-hoc JavaScript implementation for supporting the re-ordering functionality in page listing views.

It would be great to have this implementation migrated to Stimulus so that it can easily be attached to dynamically injected HTML content and be a more flexible and testable solution.

Migrating this over will also mean that we have more compliant (for CSP) implementation.

Describe the solution you'd like

Current implementation

  1. wagtail/admin/templates/wagtailadmin/pages/index.html.html - Contains the JavaScript implementation for ordering/sorting
  2. wagtail/admin/templates/wagtailadmin/pages/listing/_ordering_cell.html - Adds the column for the 'handle'.
Screenshot 2023-09-18 at 11 05 11 am

Proposed implementation

The JavaScript will need to be ported to a Stimulus controller, the code can be migrated across in the same way or possibly use a library like sortable.js.

Please do some investigation and suggest an approach before preparing a PR.

We will need a way to hook in the right data attributes.

  1. form needs the data-controller <form id="page-reorder-form" {% if ordering == 'ord' %}data-controller="w-sortable"{% endif %}> could be a starting approach.
  2. We will also need to support the suitable data attributes on the handle <div class="handle" tabindex="0" aria-live="polite" data-w-sortable-target="handle" data-action="??#w-sortable#something">
  3. There may need to be some way to nicely pass in some translations for this content.

We must consider screen reader usage, as per the current state.

Describe alternatives you've considered

  • Leave code as is and just pull out to a script file.

Additional context

@lb-
Copy link
Member Author

lb- commented Sep 18, 2023

@abhinav700 - please review the current code and have a full read of the Stimulus documentation https://stimulus.hotwired.dev/ & the Sortable.js documentation (if we go that direction) https://github.com/SortableJS/Sortable

Have a think about how to approach this and write a detailed comment with your implementation suggestion. From here we can discuss on this issue before you start building it in full.

@lb-
Copy link
Member Author

lb- commented Sep 18, 2023

@thibaudcolas or @laymonage - Would love your input on this, do we have any aversion to use a third party library for drag/drop code such as https://github.com/SortableJS/Sortable

Do you feel that we should consider anything as part of this change?

@abhinav700
Copy link

abhinav700 commented Sep 18, 2023

@lb- I'm writing to let you know that unfortunately, I will not be able to contribute to the issue we discussed yesterday. My availability has changed, and I can no longer dedicate time to this project.11
(After reading through the documentation more thoroughly, I have realized that the project demands more time and skillset than I have right now.)

I sincerely apologize for the inconvenience.

@lb-
Copy link
Member Author

lb- commented Sep 18, 2023

No problems at all @abhinav700 - there may be some simpler issues if you have time in the future. Feel free to reach out on Slack - new-contributors channel.

@salty-ivy
Copy link
Member

@lb- I think I can pick this up.

@lb-
Copy link
Member Author

lb- commented Sep 24, 2023

@salty-ivy cheers, let me know if you have questions. Thank you.

Here's a very old POC I did using Sortable JS - may come in handy (ignore all event dispatching).

https://github.com/lb-/stimulus-starter/blob/main/src/controllers/sortable-controller.js
https://github.com/lb-/stimulus-starter/blob/main/public/kanban.html

Also a reminder that the current code has a few text values for screen readers, these should be passed in as translations from the html if possible. Also, the screen reader instructions and keyboard only interaction should be preserved.

@lb-
Copy link
Member Author

lb- commented Nov 2, 2023

Some additional links.

Stimulus-sortable - a Stimulus wrapper around sortable js - https://github.com/stimulus-components/stimulus-sortable/blob/master/src/index.ts

Pragmatic Drag & Drop - an alpha library but still interesting - provides the basics for drag & drop support https://www.npmjs.com/package/@atlaskit/pragmatic-drag-and-drop

@salty-ivy
Copy link
Member

salty-ivy commented Nov 2, 2023

looking into it, apologies for the delay

@salty-ivy
Copy link
Member

salty-ivy commented Nov 14, 2023

@lb regarding the logic for ordering only via keyboard interactions which is here https://github.com/wagtail/wagtail/blob/b14bc33cdc93b06257d660256e410dada3beb6ad/wagtail/admin/templates/wagtailadmin/pages/index.html#L85C6-L85C6

these keyboard only interactions doesn't seems to work, I tried testing those firefox and chrome.

@lb-
Copy link
Member Author

lb- commented Nov 14, 2023

@salty-ivy very odd. Maybe it's not working or broke since it was implemented.

Keyboard support for reordering was added in #8972 as a fix for #5410

The idea is you are meant to be able to press enter then use up/down to move. Once you press enter a second time, the item should move to the new position.

@laymonage
Copy link
Member

laymonage commented Nov 14, 2023

It does work, you need to press tab until you arrive at the drag-and-drop grip. Then, press enter, use the arrow keys, and press enter again. You won't see the results in real time when you press the arrow keys. It only changes when you press enter to finalise the ordering.

@salty-ivy
Copy link
Member

salty-ivy commented Nov 14, 2023

@laymonage @lb- yup but, it doesn't visually tell us that row is moving up or down

lb- pushed a commit to salty-ivy/wagtail that referenced this issue Jan 18, 2024
lb- pushed a commit to salty-ivy/wagtail that referenced this issue Jan 18, 2024
lb- pushed a commit to salty-ivy/wagtail that referenced this issue Jan 18, 2024
@lb- lb- closed this as completed in 735aaa9 Jan 18, 2024
@zerolab zerolab added this to the 6.0 milestone Jan 18, 2024
lb- added a commit to lb-/wagtail that referenced this issue Jan 19, 2024
- Fix edge case where keyboard ordering and then drag drop ordering could get out of sync
- Ensure the sortable instance is cleaned up if the controller gets removed
- Relates to PR wagtail#11250 & Issue wagtail#10909
lb- added a commit to lb-/wagtail that referenced this issue Jan 19, 2024
- Fix edge case where keyboard ordering and then drag drop ordering could get out of sync
- Ensure the sortable instance is cleaned up if the controller gets removed
- Relates to PR wagtail#11250 & Issue wagtail#10909
lb- added a commit that referenced this issue Jan 20, 2024
- Fix edge case where keyboard ordering and then drag drop ordering could get out of sync
- Ensure the sortable instance is cleaned up if the controller gets removed
- Relates to PR #11250 & Issue #10909
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

No branches or pull requests

5 participants