Problem/Motivation

From lauriii on #3087685-43: Remove deprecated jQuery UI components and fork remaining source code into core

+++ /dev/null
index fd04188ab8..0000000000
--- a/core/assets/vendor/jquery.ui/README.md

Should we replace this with our own README.md? It would be great if we could mention why we have copied the files and how to generate the minified files.

Proposed resolution

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

catch created an issue. See original summary.

lauriii’s picture

Status: Active » Needs review
FileSize
1.67 KB

Any thoughts on something like this?

bnjmnm’s picture

Status: Needs review » Needs work

Two minor things and I'd be happy with this. Getting this in could mitigate potential confusion.

  1. +++ b/core/assets/vendor/jquery.ui/README.txt
    @@ -0,0 +1,47 @@
    +In preparation to potential end-of-life of jQuery UI before the end-of-life of
    +Drupal 9, code was forked here to to make it easier for us to maintain the code
    +until the end-of-life of Drupal 9. The scope of our support is limited to
    

    This first sentence is a little tough to read. Here's a possible rephrasing:

    JQueryUI will potentially reach end-of-life before Drupal 9 does. In preparation for this, Drupal has forked jQueryUI, including only the components still used by core. This fork will make it easier to maintain jQueryUI's code when necessary.
  2. +++ b/core/assets/vendor/jquery.ui/README.txt
    @@ -0,0 +1,47 @@
    +security issues and critical bugs. The fork contains only components that are
    

    It could be useful to list the components here.

swatichouhan012’s picture

Status: Needs work » Needs review
FileSize
1.58 KB
976 bytes

Updated patch wrt comment #3.1

bnjmnm’s picture

Status: Needs review » Needs work
Issue tags: +beta target

Tagging beta target. Discussed with @xjm and agreed this is an acceptable change in the beta window as this is the type of change that could go in a patch release. https://www.drupal.org/core/d8-allowed-changes

Setting back to needs work. #4 only takes care of #3.1. #3.2 still needs to be addressed.

Neslee Canil Pinto’s picture

Neslee Canil Pinto’s picture

FileSize
744 bytes
lauriii’s picture

Status: Needs review » Needs work
  1. +++ b/core/assets/vendor/jquery.ui/README.txt
    @@ -0,0 +1,57 @@
    +JQueryUI will potentially reach end-of-life before Drupal 9 does. In preparation
    ...
    +jQueryUI components used:
    

    Nit: s/JQueryUI/jQuery UI

  2. 
    +++ b/core/assets/vendor/jquery.ui/README.txt
    @@ -7,8 +7,19 @@
    +  * Draggable
    +  * Resizable
    +  * Autocomplete
    +  * Button
    +  * Checkboxradio
    +  * Controlgroup
    +  * Dialog
    +  * Menu
    +  * Position
    +  * Widget Factory
    

    Should we order these alphabetically?

Neslee Canil Pinto’s picture

Status: Needs work » Needs review
FileSize
1.76 KB
745 bytes
Neslee Canil Pinto’s picture

lauriii’s picture

Status: Needs review » Needs work
+++ b/core/assets/vendor/jquery.ui/README.txt
@@ -0,0 +1,57 @@
+for this, Drupal has forked jQueryUI, including only the components still used
+by core. This fork will make it easier to maintain jQueryUI's code when necessary.

I didn't notice these earlier but we should update jQuery UI to the correct format on these lines too

Neslee Canil Pinto’s picture

Status: Needs work » Needs review
FileSize
1.76 KB
1.04 KB
Neslee Canil Pinto’s picture

Sorry for a lot of patches, was confused with the JQueryUI and jQuery UI

longwave’s picture

Status: Needs review » Reviewed & tested by the community

This looks OK to me. We tell developers why we have done this, how to compile the code, and link to some relevant issues for more information. We don't need to say any more in this file as far as I can see.

jungle’s picture

FileSize
1.76 KB
1019 bytes
+++ b/core/assets/vendor/jquery.ui/README.txt
@@ -0,0 +1,57 @@
+jQuery UI will potentially reach end-of-life before Drupal 9 does. In preparation
...
+by core. This fork will make it easier to maintain jQuery UI's code when necessary.

Exceed 80 chars. Keep as RTBC.

Version: 8.9.x-dev » 9.1.x-dev

Drupal 8.9.0-beta1 was released on March 20, 2020. 8.9.x is the final, long-term support (LTS) minor release of Drupal 8, which means new developments and disruptive changes should now be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

xjm’s picture

Assigned: Unassigned » xjm

Reviewing this.

xjm’s picture

Version: 9.1.x-dev » 8.9.x-dev
Status: Reviewed & tested by the community » Needs work

Nice work on this.

  1. +++ b/core/assets/vendor/jquery.ui/README.txt
    @@ -0,0 +1,58 @@
    +preparation for this, Drupal has forked jQuery UI, including only the components
    +still used by core. This fork will make it easier to maintain jQuery UI's code
    

    The last part of this sentence is a little bit confusing. I'd say instead:

    Drupal has forked the jQuery UI components still used by Drupal core.

  2. +++ b/core/assets/vendor/jquery.ui/README.txt
    @@ -0,0 +1,58 @@
    +jQuery UI components used:
    +  * Autocomplete
    +  * Button
    +  * Checkboxradio
    +  * Controlgroup
    +  * Draggable
    +  * Dialog
    +  * Menu
    +  * Position
    +  * Resizable
    +  * Widget Factory
    

    We should also mention jQuery UI core here, right? Or whatever the base library is called officially. (Since there's at least that one tabbable dependency on the base library.)

  3. +++ b/core/assets/vendor/jquery.ui/README.txt
    @@ -0,0 +1,58 @@
    +Development on jQuery UI is only open for security fixes and critical bugs.
    

    I would make this more restrictive still:

    Development on this fork of jQuery UI is limited to fixes for security issues affecting Drupal projects.

  4. +++ b/core/assets/vendor/jquery.ui/README.txt
    @@ -0,0 +1,58 @@
    + * See the Drupal.org issue that partially forked jQuery UI
    ...
    + * See the Drupal.org issue for removing rest of the jQuery UI components
    

    Nitpicks: I'd add a : at the end of each of these lines. I'd also replace "See" with Read" in each sentence.

Aside from those small changes, this looks good to me! Setting to 8.9.x, because as a docs improvement this is eligible to backport to the earliest branch that includes the forks (which I think is 8.9.x.)

xjm’s picture

Version: 8.9.x-dev » 9.0.x-dev
Assigned: xjm » Unassigned

My aside was wrong; the forked sources are only in 9.0.x.

jungle’s picture

Status: Needs work » Needs review
FileSize
1.81 KB
1.71 KB

@xjm, thanks!

Addressing #18.

One change to note:

Combined #18.1 with #18.2 as listing jQuery UI core as component is not applicable IMHO. And I have not found a proper/better name for jQuery UI core, so just use it as it is.

In preparation for this, Drupal has forked the jQuery UI. jQuery UI core and jQuery
UI components still used by Drupal core.
jungle’s picture

Status: Needs review » Needs work
+++ b/core/assets/vendor/jquery.ui/README.txt
@@ -6,9 +6,9 @@ marked as an emeritus project by the OpenJS foundation. Emeritus projects are
+UI components still used by Drupal core.This fork will make it easier to

one space required right after Drupal core.

jungle’s picture

Status: Needs work » Needs review
FileSize
1.81 KB
690 bytes

Addressing #21

dww’s picture

Status: Needs review » Needs work

Mostly looking good, thanks! 2 final nits/concerns:

  1. +++ b/core/assets/vendor/jquery.ui/README.txt
    @@ -0,0 +1,60 @@
    +jQuery UI will potentially reach end-of-life before Drupal 9 does. In
    +preparation for this, Drupal has forked the jQuery UI. jQuery UI core and jQuery
    +UI components still used by Drupal core. This fork will make it easier to
    

    This is still wonky. Need to combine the middle 2 sentences:

    jQuery UI will potentially reach end-of-life before Drupal 9 does. In preparation for this, Drupal has forked jQuery UI core and the jQuery UI components still used by Drupal core. ...

  2. +++ b/core/assets/vendor/jquery.ui/README.txt
    @@ -0,0 +1,60 @@
    + * See the Drupal.org issue for removing rest of the jQuery UI components:
    

    s/removing rest/removing the rest/

jungle’s picture

Status: Needs work » Needs review
FileSize
1.8 KB
1.24 KB

Thank you @dww. A new patch addressing #23

dww’s picture

Status: Needs review » Reviewed & tested by the community

Fantastic. This is now ready. Testbot results are basically meaningless for a README-only change. If the patch didn't apply, we'd already know. ;)

Thanks!
-Derek

  • xjm committed 214fb25 on 9.1.x
    Issue #3093173 by Neslee Canil Pinto, jungle, swatichouhan012, lauriii,...

  • xjm committed a9e5966 on 9.0.x
    Issue #3093173 by Neslee Canil Pinto, jungle, swatichouhan012, lauriii,...
xjm’s picture

Status: Reviewed & tested by the community » Fixed
+++ b/core/assets/vendor/jquery.ui/README.txt
@@ -0,0 +1,60 @@
+affecting Drupal projects.
+
+
+Production versions of jQuery UI code can be generated with the following

There's an extra blank line here. Fixed on commit:

diff --git a/core/assets/vendor/jquery.ui/README.txt b/core/assets/vendor/jquery.ui/README.txt
index 0c0a63a62a..9681d0b342 100644
--- a/core/assets/vendor/jquery.ui/README.txt
+++ b/core/assets/vendor/jquery.ui/README.txt
@@ -27,7 +27,6 @@ Development
 Development on this fork of jQuery UI is limited to fixes for security issues
 affecting Drupal projects.
 
-
 Production versions of jQuery UI code can be generated with the following
 commands:

Committed to 9.1.x and 9.0.x. Thanks!

Status: Fixed » Closed (fixed)

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