Problem/Motivation

We had a desire to default the overlay of the siteimprove to be collapsed. I had implemented it for our site and thought maybe others might want it.

Steps to reproduce

Apply patch below, go to Siteimprove config (/admin/config/system/siteimprove) and scroll to bottom and select the collapse option for the overlay. It is important to note that I did this on drupal core 9.2.10. Per https://www.drupal.org/node/3104677 the js-cookie library I utilized may not work prior to 9.0.0

Remaining tasks

Review/Test

CommentFileSizeAuthor
#18 3255035-siteimprove-overlay-default-18.patch4.31 KBbartvig
#12 3255035-siteimprove-overlay-default-12.patch4.19 KBbartvig
#11 3255035-siteimprove.patch4.08 KBj-barnes
#11 interdiff_#7-#9.txt829 bytesj-barnes
#9 3255035-siteimprove.patch4.07 KBj-barnes
#9 interdiff_#7-#9.txt828 bytesj-barnes
#7 add_overlay_default-3.patch4.07 KBbilldaff
#5 siteimprove.gif79.57 KBj-barnes
#3 add_overlay_default-2.patch4.61 KBbilldaff
add_overlay_default.patch4.92 KBbilldaff

Comments

billdaff created an issue. See original summary.

beltofte’s picture

beltofte
Primary language Danish
Location Copenhagen 🇩🇰
commented
Status: Needs review » Active

Thanks for the feature request. We will be happy to add this feature, but we will need to support D8 till the release D10. Lets rework the patch to use the core/jquery.cookie backwards-compatible shim as described in https://www.drupal.org/node/3104677. The core/jquery.cookie shim will first be removed in D10.

billdaff’s picture

billdaff commented
StatusFileSize
new add_overlay_default-2.patch4.61 KB

Sure, that makes sense. Attached should work in drupal 8, it works the same still in Drupal 9

billdaff’s picture

billdaff commented
Status: Active » Needs review
j-barnes’s picture

j-barnes commented
StatusFileSize
new siteimprove.gif79.57 KB

Thanks for the patch and great idea! I ran into issues with the Siteimprove widget not saving the state (I am using Drupal 8). I think this may be related to the cookie, but I've added a GIF of the issue.

A few other things that I've noticed and suggested changes:

It looks like an extra semicolon was added to the below line in the siteimprove.module
$toolbar_items['siteimprove_toolbar']['tray']['#attached']['drupalSettings']['siteimprove']['overlay_default_collapse'] = $config->get('overlay_default_collapse');;

The below is now formatted on the same line, any reason for this change?
$prepublish_enabled = $config->get('prepublish_enabled');$enabled_content_types = $config->get('enabled_content_types');

Also, in the SettingsForms.php, I would change everything to single quote for consistency and Drupal standards.

$form['overlay']['description'] = [
  '#markup' => "<p>" . $this->t("Siteimprove overlay settings.") . "</p>",
];

SiteImprove

j-barnes’s picture

j-barnes commented
Status: Needs review » Needs work
billdaff’s picture

billdaff commented
StatusFileSize
new add_overlay_default-3.patch4.07 KB

Thanks @j-barnes for the testing and input. Find attached your suggested changes and a fix to the cookie. I had to pass in path to create the cookie. I looked at the overlay.js code they use and see that they don't specifically set this anywhere except as a default '=/' so I followed what they did so the cookie gets overwritten and handled by the overlay.js script from then on. The same line change was a mistake, I reverted it, good catch!

Find attached the updated patch.

billdaff’s picture

billdaff commented
Status: Needs work » Needs review
j-barnes’s picture

j-barnes commented
StatusFileSize
new interdiff_#7-#9.txt828 bytes
new 3255035-siteimprove.patch4.07 KB

@billdaff
This is working really well. I've attached a new patch with just a few minor tweaks. Awesome work.

  • Removing the console log
  • Setting up a constant for the '6' value
j-barnes’s picture

j-barnes commented
j-barnes’s picture

j-barnes commented
StatusFileSize
new interdiff_#7-#9.txt829 bytes
new 3255035-siteimprove.patch4.08 KB
bartvig’s picture

bartvig commented
StatusFileSize
new 3255035-siteimprove-overlay-default-12.patch4.19 KB

The patch from #11 doesn't work for me.

I have made a few changes to it to make changing the settings work, so that the overlay's collapsed status reflects the settings.

@j-barnes and @billdaff, can you check that your patch is working as expected when you change the settings, and that this patch works for you?

bartvig’s picture

bartvig commented
Status: Needs review » Needs work

  • bartvig committed 0a7a2c7 on 8.x-1.x 
    Issue #3255035 by billdaff, j-barnes, bartvig: Siteimprove Overlay...
billdaff’s picture

billdaff commented

The patch works for me in D9, however I think we have different ideas for this functionality.

It looks like your latest change will always collapse the overlay on page load if the setting is selected. My thought for this was to keep the functionality you had with storing open/close in the cookie. The scenario(s) in my head are as follows:
- Site has overlay setting collapsed by default (checked)
- User goes to a node page, sees the overlay collapsed and clicks to open it.
- When User refreshes page, or goes to a different node page, the overlay would remain open.
- If User clicks to hide it again, then one page load/visit they would see the overlap collapsed.
- This setting would remain as long as the cookie exists in browser. If cleared or expired then it would default to the overlap setting.

The latest change seems to just always collapse the overlay if the setting is set. Is this way you want this feature to work?

if(!$.cookie('sicmsplugin') || $.cookie('sicmsplugin') !== OVERLAY_COLLAPSED){
  $.cookie('sicmsplugin', OVERLAY_COLLAPSED, { domain: document.domain , path : '/'});
}

I think it will always fall into here if the setting is set, and not collapsed. I think just by changing the statement to check if we don't have a cookie would get it to the scenario above if we wanted that. Is overlay set > Do we have an existing cookie? No, then use the collapse value. Yes (we have a cookie) then stick with that.

j-barnes’s picture

j-barnes commented

@billdaff - The functionality you mention is pretty much the best of both worlds and would be preferable by our team as well.

bartvig’s picture

bartvig commented

That's a good point, @billdaff. I'll look into it.

bartvig’s picture

bartvig commented
StatusFileSize
new 3255035-siteimprove-overlay-default-18.patch4.31 KB

This patch checks if the cookie is set. If it's not, it uses the default from settings, i.e. if it's set -> collapse, if it's not set -> open.

bartvig’s picture

bartvig commented

Added to version 1.12.

bartvig’s picture

bartvig commented
Status: Needs work » Fixed

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.