I discovered today that in addition to the dialogue itself, we have a fairly large amount of jquery code.
I believe that in order to make switching to a native dialog easier in the future, we need to get rid of jQuery that is not related to the functionality of the dialog.
In this task I simplified the code in dialog.ajax.js
Now there are only $ associated with the jQueryUI dialog options.
probably we need to add change record for `dialogButtonsChange` Event.
| Comment | File | Size | Author |
|---|---|---|---|
| #25 | 3460979-nr-bot.txt | 1.51 KB | needs-review-queue-bot |
| #23 | 3460979-nr-bot.txt | 1.51 KB | needs-review-queue-bot |
| #21 | 3460979-nr-bot.txt | 1.51 KB | needs-review-queue-bot |
Show commandsStart within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
changes, plain diff MR !8751compare 
Comments
Comment #2
finnsky commentedComment #5
finnsky commentedComment #6
finnsky commentedComment #8
vladimirausLooks good.
Comment #9
nod_I like it, we do need to add the deprecation of the jQuery event here. And introduce a BC layer for events so that calling
$dialog.trigger('event');triggers a deprecation and generate the custom DOM event.Comment #10
finnsky commentedAdded BC and change record
Comment #11
smustgrave commentedPossible to get a deprecation test.
Comment #12
finnsky commentedNot sure how this deprecation check work in Drupal.
I wrote Nightwatch test. But is seems still not working as it should.
https://gyazo.com/825ac43bc3dea0bb303cc214a7fab597
Depreciation messages presented in console. script js aswell.
Comment #13
finnsky commentedThat is weird. But in logs i see that test was trigered in test environment.
Comment #14
finnsky commentedComment #15
smustgrave commentedJS skill is mid at best.
But deprecation and test coverage looks correct to me.
Testing the changes on dialog popups on views and block_layout and everything appears to be working as before.
Going to go on a limb and say this is good.
Comment #16
finnsky commentedrebased, seems ready to go
Comment #17
finnsky commentedComment #18
finnsky commentedRebased. Review needed
¯\_(ツ)_/¯Comment #19
nod_Thanks, solid work, still a few things to address. Thanks for your patience!
Comment #20
finnsky commentedI've added one comment about jQuery selectors.
All other feedbacks are fixed.
Comment #21
needs-review-queue-bot commentedThe Needs Review Queue Bot tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".
This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.
Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.
Comment #22
finnsky commented@nod_ thank you for feedbacks. i fixed everything according them.
I agree - we need to care about jquery selectors in own task and deprecate them globally at once.
please review!
Comment #23
needs-review-queue-bot commentedThe Needs Review Queue Bot tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".
This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.
Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.
Comment #24
finnsky commentedComment #25
needs-review-queue-bot commentedThe Needs Review Queue Bot tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".
This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.
Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.
Comment #26
nod_Comment #27
smustgrave commentedBelieve needs a manual rebase now, probably would update the deprecations for 11.2 now :(
Comment #28
finnsky commentedrebased and updated versions
Comment #29
smustgrave commentedSorry to do it but appears to need another rebase.
Comment #30
finnsky commentedRebased. Please review
Comment #31
smustgrave commentedSorry slipped my radar, needs a rebase again. Please ping me so I can add to top of my list
If you are another contributor eager to jump in, please allow the previous poster at least 48 hours to respond to feedback first, so they have the opportunity to finish what they started!
Comment #32
vladimiraus+1 for RTBC.
Comment #33
smustgrave commentedSeems to have test failures now though, functional, javascript, and nightwatch
Comment #34
finnsky commented@vladimiraus thank you for care about this issue ;)
@smustgrave here usual random failures. all green now.
Comment #35
smustgrave commentedSeems to need another rebase
@finnsky or @VladimirAus feel free to self RTBC after
If you are another contributor eager to jump in, please allow the previous poster at least 48 hours to respond to feedback first, so they have the opportunity to finish what they started!
Comment #36
finnsky commentedRebased. Thank you Please review.
Comment #37
smustgrave commentedSeems good now
Comment #38
finnsky commentedRebased. Please review
Comment #39
catchThis looks like good clean-up and a nice net reduction in code even without taking into a account the new test coverage, but it needs someone more js-competent than me to do a final review so tagging for front end framework managers.
Comment #41
nod_:)
Committed bd70656 and pushed to 11.x. Thanks!
Comment #42
nod_Comment #44
acbramley commentedThe test here caused a very frequent random fail #3521621: [random test failure] jQuery Events Deprecation Tests (Tests/dialogDeprecations) let's try to a) be careful with random ms waits in tests (wait for elements instead) and b) not use nightwatch wherever possible.