Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
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
Comment | File | Size | Author |
---|---|---|---|
#24 | interdiff-22-24.txt | 1.24 KB | jungle |
#24 | 3093173-24.patch | 1.8 KB | jungle |
#12 | 3093173-12.patch | 1.76 KB | Neslee Canil Pinto |
#10 | interdiff_4-10.txt | 983 bytes | Neslee Canil Pinto |
#10 | 3093173-10.patch | 1.76 KB | Neslee Canil Pinto |
Comments
Comment #2
lauriiiAny thoughts on something like this?
Comment #3
bnjmnmTwo minor things and I'd be happy with this. Getting this in could mitigate potential confusion.
This first sentence is a little tough to read. Here's a possible rephrasing:
It could be useful to list the components here.
Comment #4
swatichouhan012 CreditAttribution: swatichouhan012 at Valuebound for Valuebound commentedUpdated patch wrt comment #3.1
Comment #5
bnjmnmTagging 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.
Comment #6
Neslee Canil PintoComment #7
Neslee Canil PintoComment #8
lauriiiNit: s/JQueryUI/jQuery UI
Should we order these alphabetically?
Comment #9
Neslee Canil PintoComment #10
Neslee Canil PintoComment #11
lauriiiI didn't notice these earlier but we should update jQuery UI to the correct format on these lines too
Comment #12
Neslee Canil PintoComment #13
Neslee Canil PintoSorry for a lot of patches, was confused with the JQueryUI and jQuery UI
Comment #14
longwaveThis 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.
Comment #15
jungleExceed 80 chars. Keep as RTBC.
Comment #17
xjmReviewing this.
Comment #18
xjmNice work on this.
The last part of this sentence is a little bit confusing. I'd say instead:
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.)
I would make this more restrictive still:
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.)
Comment #19
xjmMy aside was wrong; the forked sources are only in 9.0.x.
Comment #20
jungle@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.
Comment #21
jungleone space required right after
Drupal core.
Comment #22
jungleAddressing #21
Comment #23
dwwMostly looking good, thanks! 2 final nits/concerns:
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. ...
s/removing rest/removing the rest/
Comment #24
jungleThank you @dww. A new patch addressing #23
Comment #25
dwwFantastic. 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
Comment #28
xjmThere's an extra blank line here. Fixed on commit:
Committed to 9.1.x and 9.0.x. Thanks!