Once the sortable library is in 8.8 and we can rely on it to exist on testbot, we should use that.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Berdir created an issue. See original summary.

Berdir’s picture

Issue summary: View changes

Blocked on #3064049: Replace jQuery UI sortable with Sortable js, once that is in we can start with a patch to test it.

Martijn de Wit’s picture

bartlangelaan’s picture

Status: Active » Needs review
FileSize
2.37 KB

This patch adds core/sortable as a library dependency to core/paragraphs-dragdrop. When core/sortable is not available (8.7 or older), the old behaviour is invoked.

This means that from 8.8 onwards, the core version of sortable is always used.

Because this change makes the drag-and-drop functionality work out-of-the-box with Drupal 8.8+, can we maybe split this issue up and create tests in a separate issue? This patch doen't change and/or improve anything on a functional level.

bartlangelaan’s picture

Berdir’s picture

Status: Needs review » Needs work
  1. +++ b/paragraphs.module
    @@ -391,10 +391,23 @@ function paragraphs_preprocess_field_multiple_value_form(&$variables) {
     function paragraphs_libraries_info() {
    +  if (_paragraphs_sortable_in_core()) return [];
    

    short if syntax isn't allowed in the drupal coding standard.

  2. +++ b/paragraphs.module
    @@ -426,6 +439,18 @@ function paragraphs_library_info_alter(&$libraries, $extension) {
     
    +  if (_paragraphs_sortable_in_core()) {
    +    // When core/sortable is available, the paragraphs/sortable library can be removed.
    +    unset($libraries['sortable']);
    +    return $libraries;
    +  }
    +  else {
    

    Lets do this with a version check, that should be easier, this almost recursively discovers libraries which is a bit scary:

    if (version_compare(\Drupal::VERSION, '8.8', '<')) {
    

For tests, lets make a deal, you create a follow-up to add test coverage and then I'll commit it without ;)

bartlangelaan’s picture

Title: Use sortable library from core and add JS test coverage for drag and drop » Use sortable library from core
Issue summary: View changes
FileSize
2.11 KB

See adjusted patch.

I adjusted the issue title & summary. Also, created a follow-up: #3099706: Add JS test coverage for drag and drop.

bartlangelaan’s picture

Status: Needs work » Needs review
Berdir’s picture

Status: Needs review » Fixed

Tested that in our project, works well. And once we require 8.8 we can just remove all that code. Also updated the README.txt a bit.

  • Berdir committed 6d80300 on 8.x-1.x authored by bartlangelaan
    Issue #3090457 by bartlangelaan, Berdir: Use sortable library from core
    

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.