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>

Comments

RajabNatshah created an issue. See original summary.

RajabNatshah’s picture

When we do not have it too.

droplet’s picture

Component: ajax system » outside_in.module
Status: Needs work » Active
Issue tags: +JavaScript

Needs 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

RajabNatshah’s picture

tedbow’s picture

@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.

RajabNatshah’s picture

Thanks 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 :)

tedbow’s picture

Status: Active » Postponed (maintainer needs more info)

@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.

tkoleary’s picture

@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.

RajabNatshah’s picture

Than you Terrence,

We had this fixed for Bootstrap at this issue.

#2821008: [bootstrap] Add support for the Settings Tray (outside_in) experiemental module

Left to right with Bootstrap
outside in ltr

Right to left with Bootstrap
outside in rtl

The patch is undergoing under the issue #2821008: [bootstrap] Add support for the Settings Tray (outside_in) experiemental module

tedbow’s picture

Status: Postponed (maintainer needs more info) » Closed (duplicate)
Related issues: +#2826726: Settings tray will not trigger in Bootstrap
droplet’s picture

Status: Closed (duplicate) » Active

Needs a simple check on #main-canvas.
e.g.
if(!$('#main-canvas').length) { return; }

Or throw a compatibility warning to help themers

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.3.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

tedbow’s picture

Status: Active » Needs review
FileSize
790 bytes

We 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

Needs a simple check on #main-canvas.
e.g.
if(!$('#main-canvas').length) { return; }

Or throw a compatibility warning to help themers

Since we are not using a class which might be changed and warning about the attribute I think this is not needed anymore.

drpal’s picture

Version: 8.3.x-dev » 8.4.x-dev
Status: Needs review » Reviewed & tested by the community

I agree that the comment is probably the way to go here.

webchick’s picture

Assigned: Unassigned » droplet
Status: Reviewed & tested by the community » Needs review

Assigning to droplet for his 2 cents.

droplet’s picture

Assigned: droplet » Unassigned

In 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?

drpal’s picture

Alright, this throws an error the start of setEditModeState() if you've removed data-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.

tedbow’s picture

Status: Needs review » Reviewed & tested by the community

re #16

In my opinion, it still worth to stop the error.

#17 covers this. I test it does through a Javascript console error.

(I think it will allow to opt-out to use Setting Tray for some pages for whatever reasons)

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.

It has another problem. When the [data-offcanvas-main-canvas] is missing, the top toolbar "EDIT" button still there. It leads further errors.

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.

So the whole point is: Do we allow [data-offcanvas-main-canvas] is missing when the module is enabled?

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!

droplet’s picture

OK. So we have a warning from JS side now.

I think the warning in the twig file is sufficient.

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 would say no and but if that would be necessary if should be different issue.

I think it again. It sounds right. If you want to opt-out, then you should hide the EDIT button instead.

Thanks!

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 17: 2811717-17.patch, failed testing.

tedbow’s picture

Status: Needs work » Reviewed & tested by the community

Drupal CI error

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 17: 2811717-17.patch, failed testing.

himanshu-dixit’s picture

Status: Needs work » Reviewed & tested by the community

This looks good to me. Setting back to RTBC again

alexpott’s picture

Status: Reviewed & tested by the community » Needs work

I'm concerned that this is RTBC and #2830882: Abstract outside_in.js (Settings Tray) JS to be jQuery UI independent is postponed. #2830882: Abstract outside_in.js (Settings Tray) JS to be jQuery UI independent 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: Abstract outside_in.js (Settings Tray) JS to be jQuery UI independent.

dawehner’s picture

I 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.

tedbow’s picture

@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