Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
The edit module is using a modal dialog that is not the core provided one.
Comment | File | Size | Author |
---|---|---|---|
#21 | Screen Shot 2013-08-28 at 1.55.29 AM.png | 31.84 KB | webchick |
#18 | edit_dialog-1872296-18.patch | 11.54 KB | Wim Leers |
#18 | interdiff.txt | 957 bytes | Wim Leers |
#17 | interdiff.txt | 3.3 KB | nod_ |
#16 | interdiff.txt | 3.3 KB | nod_ |
Comments
Comment #1
Wim LeersBecause it was not (and AFAIK still is not) ready. Is that no longer correct?
Comment #2
Bojhan CreditAttribution: Bojhan commentedYou should just implement it, as far as I know.
Comment #3
swentel CreditAttribution: swentel commentedSee http://drupal.org/node/1852224 and http://drupal.org/node/1853388
Comment #4
Wim Leers#3: thanks, but I was aware of those :) I guess I have the impression that it's not yet ready because nothing in core is using it yet. Or is that just because we have only rarely the need to use a dialog?
Comment #5
swentel CreditAttribution: swentel commentedNo idea, we sometimes commit stuff and do nothing with it .. :)
Comment #6
Bojhan CreditAttribution: Bojhan commented@Wimleers A chicken/egg problem :)
Comment #7
Wim Leers.
Comment #8
Wim LeersNow that the dialog API has matured and #1678002: Edit should provide a usable entity-level toolbar for saving fields has landed, we should start working on this!
Comment #9
Wim LeersSo it is unfortunately impossible to create a consistent experience right now: #2071801: Client-side generated dialogs provide an inconsistent experience.
But, that doesn't mean we can't move forward here. We can at least get rid of Edit's custom modal, because at least we'll be using the same dialog system.
Another related, but out-of-scope bug is #2067285: The entity in-place editing toolbar appears above the Drupal dialog.
30 insertions, 163 deletions!
Comment #10
jessebeach CreditAttribution: jessebeach commentedWe're tracking issues to address the z-indexing issues of the Drupal core dialog. Those bugs should not be addressed in this issue which is just to get Edit using the sanctioned dialog in core, problems and all.
#2067285: The entity in-place editing toolbar appears above the Drupal dialog
#2072037: Drupal dialog modal background z-index is set too low to reliably occlude core UI components
Comment #11
Bojhan CreditAttribution: Bojhan commentedWhoooo!
Comment #12
jessebeach CreditAttribution: jessebeach commentedOne would think that the close X button in the upper right of the dialog would also invoke the close callbacks on the dialog, but this is not the case, so Wim had to hide the close button in order to prevent a user from exiting the modal without making a decision about whether to keep to throw away the changes. Hooking into the dialog close flow is proving to be really messy, so I agree with the decision to just hide the button. Oddly, the jQuery UI API docs explain how to do this as well.
I jsHinted the code changes and found three missing semi-colons. This updated patch reflects just those additional semi-colons and no other changes. Other than that, this is straightforward port to a standard UI pattern in core. I've manually tested the change and the dialog is working as expected.
As soon as the bot comes back green, we can RTBC.
Comment #13
jessebeach CreditAttribution: jessebeach commentedBot came back green and I made no significant changes with the patch in #12 to the patch from #9, so I'm setting this to RTBC.
Comment #14
tstoecklerI have no clue whatever the heck is going on this issue :-), but I can certainly approve the latest interdiff.
RTBC++
Comment #15
nod_Just removed the use of the global event. It was added to the API to alter things that all dialog would share, since here which dialog is closing is checked and the same thing can be done without the listener, I just moved a few things around.
The official HTML5 API doesn't provide those global events so we shouldn't rely on them to build dialog-specific functionality.
rerolled. As far as I can see, everything still works. bear with me until I find how to interdiff on windows…
Comment #16
nod_here it is
Comment #17
nod_now in the right way…
Comment #18
Wim Leers#12: hah! :) Thanks :)
#15: If it must be done this way, then the documentation in both the code (there is none) and the change notice is majestically bad. The change notice lists 4 events (and doesn't even say they occur on the window), and never indicates these MUST NOT be used unless for altering *all* dialogs.
Also: if the events are only supposed to serve "global alterations", then why do we even bother to pass a return value to the
dialog.close()
method and why do we even havedialog.returnValue
? Global code can't/shouldn't look atdialog.returnValue
, and most code will look like the one in this patch, having no need whatsoever fordialog.returnValue
.The patch in #15 also no longer deletes the image file *nor* the
ModalView.js
file.Reroll attached that slightly simplifies/clarifies things, and includes the deletion of the two above files again.
Comment #19
nod_returnValue is from he HTML5 spec. It's not that they must not, it's more like it isn't necessary in that case.
Sorry about the bad reroll, that's what I get for working on windows :p
#18++
Comment #20
Wim LeersAls per #13, #14, #15 and #19, back to RTBC!
Comment #21
webchickWell, that's a lovely diffstat!
The main user-facing change seems to be a darker background on the modal.
Before:
After:
Not having to maintain two copies of dialog code seems like a pretty major thing to me.
Committed to 8.x. Can't push atm because of https://drupal.org/node/2075775 but hopefully will be able to tomorrow.
Comment #22
webchickAhem.
Comment #23
Wim Leers.
Comment #24
Wim Leersd.o tag monster