Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
When we do have panelizer On!!!!
$editables = $('[data-drupal-outsidein="editable"]').once('outsidein');
if ($editables.length) {
// Use event capture to prevent clicks on links.
<strong>document.querySelector('#main-canvas').addEventListener('click', preventClick, true);</strong>
Comment | File | Size | Author |
---|---|---|---|
#29 | interdiff-17-29.txt | 1.48 KB | tedbow |
#29 | 2811717-29.patch | 1.38 KB | tedbow |
#17 | interdiff.txt | 1.04 KB | GrandmaGlassesRopeMan |
#17 | 2811717-17.patch | 1.39 KB | GrandmaGlassesRopeMan |
#13 | 2811717-13.patch | 790 bytes | tedbow |
Comments
Comment #2
Rajab Natshah CreditAttribution: Rajab Natshah at Vardot commentedWhen we do not have it too.
Comment #3
droplet CreditAttribution: droplet commentedNeeds a check on #main-canvas.
It's a PHP side bug tho. In some cases #main-canvas won't apply to (custom) theme
@RajabNatshah, can you tell us more about your Drupal site configs
Comment #4
Rajab Natshah CreditAttribution: Rajab Natshah at Vardot commentedVarbase
https://www.drupal.org/project/varbase
Comment #5
tedbow@droplet @RajabNatshah custom themes should still have because of outside_in_element_info_alter. I tried a few contrib themes and they work.
I tried vartheme which is inside varbase but the install profile itself. The #main-canvas wrapping is there.
Comment #6
Rajab Natshah CreditAttribution: Rajab Natshah at Vardot commentedThanks Ted,
Your feedback on this issue is rewarded :)
Varthem is a sub theme of bootstrap theme.
I will keep testing and troubleshooting this issue.
Rewarded time :)
Comment #7
tedbow@RajabNatshah thanks for looking into it more. I am going to postpone this issue just to keep track better.
Please re-open if you are able to figure out more.
The more you can isolate the problem the better. varbase as a distribution probably has a lot going on. So if we could figure out if is the theme or some module that is enabled during the install that would help.
Comment #8
tkoleary CreditAttribution: tkoleary at Acquia commented@RajabNatshah
Settings tray is currently not working with Bootstap or any subtheme of bootstrap because bootstrap completely removes and replaces jquery dialog. It's likely that Bootstrap will not support settings tray until it is no longer experimental.
Comment #9
Rajab Natshah CreditAttribution: Rajab Natshah at Vardot commentedThank you Terrence,
We had this fixed for Bootstrap at this issue.
#2821008: [bootstrap] Add support for the Off-canvas dialog used by Settings Tray and Layout Builder
Left to right with Bootstrap
Right to left with Bootstrap
The patch is undergoing under the issue #2821008: [bootstrap] Add support for the Off-canvas dialog used by Settings Tray and Layout Builder
Comment #10
tedbowClosing as duplicate of #2826726: Settings tray will not trigger in Bootstrap
Comment #11
droplet CreditAttribution: droplet commentedNeeds a simple check on #main-canvas.
e.g.
if(!$('#main-canvas').length) { return; }
Or throw a compatibility warning to help themers
Comment #13
tedbowWe are no longer using #main-canvas here.
We are now using an attribute data-offcanvas-main-canvas set up in outside-in-page-wrapper.html.twig.
I added a comment in outside-in-page-wrapper.html.twig warning that you cannot remove this attribute. So a themer would be warned when they copied the default template from the module to override.
Re @droplet's comment
Since we are not using a class which might be changed and warning about the attribute I think this is not needed anymore.
Comment #14
GrandmaGlassesRopeManI agree that the comment is probably the way to go here.
Comment #15
webchickAssigning to droplet for his 2 cents.
Comment #16
droplet CreditAttribution: droplet commentedIn my opinion, it still worth to stop the error. (I think it will allow to opt-out to use Setting Tray for some pages for whatever reasons)
It has another problem. When the [data-offcanvas-main-canvas] is missing, the top toolbar "EDIT" button still there. It leads further errors.
So the whole point is: Do we allow [data-offcanvas-main-canvas] is missing when the module is enabled?
Comment #17
GrandmaGlassesRopeManAlright, this throws an error the start of
setEditModeState()
if you've removeddata-offcanvas-main-canvas
from the template. One thing to keep in mind is that the edit-mode functionality is prohibited, but the dialog system effects will still run.Comment #18
tedbowre #16
#17 covers this. I test it does through a Javascript console error.
Edit mode will not work if the attribute is not there. I think per page disabling of Settings Tray functionality is beyond the scope of this issue about Javascript error.
That should be separate Feature Request issue if needed. It should also probably be solved on the server side by loading JS files all together.
I think the warning in the twig file is sufficient. If someone is removing the attribute they should see the warning in the code comment. If they don't they will then see the error in the Javascript console.
If think if they go through all that and don't want errors they need to do library alter and provide their own JS files that work without the attribute.
I would say no and but if that would be necessary if should be different issue.
Going to mark this as RTBC. Thanks all for work on this!
Comment #19
droplet CreditAttribution: droplet commentedOK. So we have a warning from JS side now.
It's never enough. I think reporter of this issue is the one who never touches the twig but gets errors. We have many developers never understand how to trace down to error source yet.
I think it again. It sounds right. If you want to opt-out, then you should hide the EDIT button instead.
Thanks!
Comment #21
tedbowDrupal CI error
Comment #23
himanshu-dixit CreditAttribution: himanshu-dixit as a volunteer and at Google Summer of Code commentedThis looks good to me. Setting back to RTBC again
Comment #24
alexpottI'm concerned that this is RTBC and #2830882: Introduce Drupal.offCanvas, mirrored after Drupal.dialog, to avoid Settings Tray using Drupal.Dialog in unintended ways is postponed. #2830882: Introduce Drupal.offCanvas, mirrored after Drupal.dialog, to avoid Settings Tray using Drupal.Dialog in unintended ways feels like a critical for outside to do before the module graduates from experimental. Before we commit this we need to clarify what is in scope for the outside module since committed this doesn't solve the original issue that cause the reporter to file it.
Marking as needs work to to get agreements as to the status of #2830882: Introduce Drupal.offCanvas, mirrored after Drupal.dialog, to avoid Settings Tray using Drupal.Dialog in unintended ways.
Comment #25
dawehnerI think what we need is a planning issue describing the steps we have to do to get it stable. According to @drpal there was some dicussion and a plan which wasn't posted yet.
Comment #26
tedbow@dawehner yes you are right. I will work on issue or point to existing about getting this module stable.
UPDATE: This issue has must haves for this to get stable #2762505: Introduce "outside in" quick edit pattern to core
Comment #27
tedbow@alexpott I agree #2830882: Introduce Drupal.offCanvas, mirrored after Drupal.dialog, to avoid Settings Tray using Drupal.Dialog in unintended ways should be resolved.
We have been working on that issue.
We have a patch https://www.drupal.org/node/2830882#comment-12068569
Getting initial feedback from @markcarver the bootstrap theme maintainer that it is generally in the right direction. It also appears that patch will not affect this problem.
I think the patch does solve the initially reported issue.
document.querySelector('#main-canvas').addEventListener('click', preventClick, true);
Was throwing an error. This was because
document.querySelector('#main-canvas')
returned no element.The selector has now been changed to '[data-offcanvas-main-canvas]' because of the other commits. But the same problem remains. It would throw an error if the template was overridden and the querySelector return no elements.
We are doing 2 things to mitigate this:
Explicitly throwing an error to make these easier to debug.
Adding a template comment to inform the themer not remove the attribute needed.
This solves the initial problem in the sense the themer would have to ignore the template comment, remove the attribute, and ignore the console error to still encounter this problem.
There is only so much we can do. Obviously if you can break this module or others if you are very careless in overriding templates.
Comment #29
tedbowTest failed because the actual attribute changed from
data-offcanvas-main-canvas
todata-off-canvas-main-canvas
in #2862625: Rename offcanvas to two words in code and comments.Fixing
Comment #30
GrandmaGlassesRopeManAlright. I think the discussion happening in #2830882: Introduce Drupal.offCanvas, mirrored after Drupal.dialog, to avoid Settings Tray using Drupal.Dialog in unintended ways shouldn't affect the status of this. With the fix for the test, I think this is good.
Comment #32
tedbowDrupalCI database server error so re-testing
Comment #33
GrandmaGlassesRopeManComment #35
lauriiiThanks @tedbow for the great summary on #27. I think this is the best way to address the original issue report. Big +1 from me for throwing useful errors helping developers to fix bugs instead of just silently failing.
Committed e67f64e and pushed to 8.4.x. Thanks!
I think this could be backported to 8.3.x since this is only affecting experimental modules and the error only appears in situations where another error would have happened anyway. This will likely need a new patch for 8.3.x because of #2862625: Rename offcanvas to two words in code and comments. .
Comment #36
lauriiiComment #37
tedbow@lauriii thanks for committing!!!!
Just tested and the patch applies cleanly to 8.3.x because #2862625: Rename offcanvas to two words in code and comments. was also applied to 8.3.x.
For now 8.3.x is receiving all the backports because we are keeping all the changes in the experimental module folder. So hopefully all patches will apply to both 8.4.x and 8.3.x
These could change with #2784443: Move off-canvas functionality from Settings tray module into drupal.dialog.ajax library so that other modules can use it but that will be one of the last things before we get the module stable.
Comment #39
lauriiiI got confirmation from @xjm that this is eligible for backporting given that this only modifies experimental modules. Cherry picked 657a612 and pushed to 8.3.x.
Comment #41
tedbowChanging to new settings_tray.module component. @drpal thanks for script help! :)