Merge Requests

More

Recent comments

๐Ÿ‡ง๐Ÿ‡ชBelgium f0ns
๐Ÿ‡ง๐Ÿ‡ชBelgium f0ns
๐Ÿ‡ง๐Ÿ‡ชBelgium f0ns
๐Ÿ‡ง๐Ÿ‡ชBelgium f0ns
๐Ÿ‡ง๐Ÿ‡ชBelgium f0ns
๐Ÿ‡ง๐Ÿ‡ชBelgium f0ns
๐Ÿ‡ง๐Ÿ‡ชBelgium f0ns

f0ns โ†’ created an issue.

๐Ÿ‡ง๐Ÿ‡ชBelgium f0ns

The fix doesn't apply anymore in the latest upgrade to Drupal 11.2.5, I'll look into it.

๐Ÿ‡ง๐Ÿ‡ชBelgium f0ns

Oops, I didn't catch this either on a clean install.

There must have been something is that was already calling core/once.

Glad it's fixed. Thanks!

๐Ÿ‡ง๐Ÿ‡ชBelgium f0ns
๐Ÿ‡ง๐Ÿ‡ชBelgium f0ns

f0ns โ†’ created an issue.

๐Ÿ‡ง๐Ÿ‡ชBelgium f0ns

I have been looking into this because it indeed would be an improvement to the module if no empty block:

<div class="js-toc-block"></div>

would be rendered.

The only way to fix this, in my opinion, is to do a DOM check the contentSelector if it has any headingSelectors and if not don't bother rendering the div.

๐Ÿ‡ง๐Ÿ‡ชBelgium f0ns
๐Ÿ‡ง๐Ÿ‡ชBelgium f0ns
๐Ÿ‡ง๐Ÿ‡ชBelgium f0ns

f0ns โ†’ created an issue.

๐Ÿ‡ง๐Ÿ‡ชBelgium f0ns
๐Ÿ‡ง๐Ÿ‡ชBelgium f0ns

Casting all properties that are numbers to an int (interpreted as a string otherwise) fixed it for me.

https://git.drupalcode.org/issue/tocbot-3547569/-/commit/4d1066e82927f51...

๐Ÿ‡ง๐Ÿ‡ชBelgium f0ns
๐Ÿ‡ง๐Ÿ‡ชBelgium f0ns

Hello thank you for the information.

I just did a quick scan and I don't see any dependencies that are stated in the Sticky module (except for using the latest version of the Sticky library).

{
    "type": "package",
    "package": {
        "name": "garand/sticky",
        "version": "1.0.4",
        "type": "drupal-library",
        "source": {
            "url": "https://github.com/garand/sticky.git",
            "type": "git",
            "reference": "1.0.4"
        }
    }
},
๐Ÿ‡ง๐Ÿ‡ชBelgium f0ns

After a lot of debugging I think I found the issue.

When I set

options.headingsOffset = 1;

in the tocbot-init.js file just before tocbot.init() everything works great. Looks like it gets passed as text instead of a number and Tocbot doesn't throw an error and just doesn't work well then.

I'll dig deeper and look for a patch but if anyone else has this issue in the meantime this information could be helpful.

๐Ÿ‡ง๐Ÿ‡ชBelgium f0ns

Tested with some older Tocbot libraries but all three instances have the same issue.

I have already debugged so much that I don't know where to look anymore to be honest.

Will look into the tocbot-init.js once more. If I don't find anything there I think a fresh pair of eyes/someone smarter than me is needed.

๐Ÿ‡ง๐Ÿ‡ชBelgium f0ns

I'm now testing with the latest Tocbot version, I perhaps could test with older versions and see if it's library specific or not to rule that out.

๐Ÿ‡ง๐Ÿ‡ชBelgium f0ns

f0ns โ†’ created an issue.

๐Ÿ‡ง๐Ÿ‡ชBelgium f0ns

I had the same error.

Applying and testing the code that is in the merge request (https://git.drupalcode.org/project/pathauto/-/merge_requests/91.patch) fixed it for me.

Thanks!

๐Ÿ‡ง๐Ÿ‡ชBelgium f0ns

Then I canโ€™t think of a case where you need to calc the height dynamically where itโ€™s not 100% of the height for the sidebar.

๐Ÿ‡ง๐Ÿ‡ชBelgium f0ns

I think it depends on how you see the sidebar vs the topbar.

Since the sidebar holds the logo I personally feel it should always be 100% height and the header shouldn't overlap it or push it down because the logo wouldn't be in the lefthand top corner in this case which feels visually weird.

๐Ÿ‡ง๐Ÿ‡ชBelgium f0ns

The suggestion indeed also fixes the problem and seems to me that this is a better solution (delete one line of CSS instead of adding one line of CSS).

Please review!

๐Ÿ‡ง๐Ÿ‡ชBelgium f0ns

Thank you sir and sorry for the extra hassle. Have a great day and thank you for the new release.

๐Ÿ‡ง๐Ÿ‡ชBelgium f0ns

You might be right, I did not notice this css at first.

Iโ€™ll test this and get back on this.

๐Ÿ‡ง๐Ÿ‡ชBelgium f0ns

Can you test with the provided patches if it also fixes it for you? Here everything works now. Thanks!

๐Ÿ‡ง๐Ÿ‡ชBelgium f0ns

@xjm it should be fixed now. Thanks!

๐Ÿ‡ง๐Ÿ‡ชBelgium f0ns

Thank you for testing, iโ€™ll leave it on needs review for now and see if we can have an extra pair of eyes on this one.

๐Ÿ‡ง๐Ÿ‡ชBelgium f0ns

I've added the css to fix this:

https://git.drupalcode.org/issue/drupal-3531516/-/commit/8fda91d0d4805a5...

I've patched my sites with it and everything works as expected now.

๐Ÿ‡ง๐Ÿ‡ชBelgium f0ns

I've added a merge request that fixes the problem for me.

The message is rendered above the sidebar, it disappears after a few seconds so this behaviour is more desirable than an unreadable message.

๐Ÿ‡ง๐Ÿ‡ชBelgium f0ns

That is weird, I just tested my initial patch and the MR and both apply correctly and fix the issue.

๐Ÿ‡ง๐Ÿ‡ชBelgium f0ns

I'm on a Drupal 11 installation where I use:

  • Conditional fields - Version: 4.0.0-alpha6
  • Layout Builder iFrame Modal - Version: 1.3.6

When creating a block with /block/add/[block-type] everything was working as expected.

When creating a block in layout builder and in the layout builder iframe modal it was not working as expected because conditional fields didn't trigger anything.

After applying and testing the patch in #38 everything works as it should. Thanks a lot for your work on this, it helped me out a lot!

๐Ÿ‡ง๐Ÿ‡ชBelgium f0ns

I'm running MAMP on Mac OS X and I found this issue which helped me.

I'm adding it here for future reference if needed:

I was unable to send mails locally (and catch them with mailhog).

I got the following error:

An attempt to send an e-mail message failed, and the following error message was returned : Error creating transports: Unsupported sendmail command flags &quot;/Applications/MAMP/Library/bin/mhsendmail --smtp-addr=0.0.0.0:1025&quot;; must be one of &quot;-bs&quot; or &quot;-t&quot; but can include additional flags.

The solution was to add the following line in the php.ini file of the active php version:

; For Unix only.  You may supply arguments as well (default: "sendmail -t -i").
; http://php.net/sendmail-path
sendmail_path = "/Applications/MAMP/Library/bin/mhsendmail -t --smtp-addr=0.0.0.0:1025";
๐Ÿ‡ง๐Ÿ‡ชBelgium f0ns

Thank you for the new release ๐Ÿ™

๐Ÿ‡ง๐Ÿ‡ชBelgium f0ns

I've tested the MR and everything works fine.

๐Ÿ‡ง๐Ÿ‡ชBelgium f0ns

The patch in attachment fixed the issue for me.

๐Ÿ‡ง๐Ÿ‡ชBelgium f0ns

This patch fixed this behavior in the settings form for me.

๐Ÿ‡ง๐Ÿ‡ชBelgium f0ns

Would be nice to get a D11 release out there. ๐Ÿ™

๐Ÿ‡ง๐Ÿ‡ชBelgium f0ns

I had the same issue and the MR fixed it for me in the form of a patch (in attachment for others that also need it).

๐Ÿ‡ง๐Ÿ‡ชBelgium f0ns

I created a patch with replaces $.trim with .trim based on the JQuery documentation:

Note: This API has been deprecated in jQuery 3.5; please use the native String.prototype.trim method instead. Unlike jQuery.trim, String.prototype.trim does not work with types other than strings (null, undefined, Number). Make sure that your code is compatible when migrating.

Source: https://api.jquery.com/jQuery.trim/#:~:text=Note%3A%20This%20API%20has%2....

๐Ÿ‡ง๐Ÿ‡ชBelgium f0ns

I have the same issues in a Drupal 11 environment using the Views Load More module with the following error:

An error occurred during the execution of the Ajax response: TypeError: $.trim is not a function"

Your reasoning on where this error comes from seems accurate.

๐Ÿ‡ง๐Ÿ‡ชBelgium f0ns

Tested and works great, would be nice to get this in an official release. Thanks!

๐Ÿ‡ง๐Ÿ‡ชBelgium f0ns

Added the correct patch and not the old one that did not apply =)

๐Ÿ‡ง๐Ÿ‡ชBelgium f0ns

I applied MR 35 as a patch:

https://git.drupalcode.org/project/context/-/merge_requests/35.patch

But then the D11 compatibility patch didn't apply anymore after that so I made a new one (in attachment) that does work.

Apply it after the above patch.

๐Ÿ‡ง๐Ÿ‡ชBelgium f0ns

Just tested the new patch on 4.2.2, it works great.

Thanks a lot!

๐Ÿ‡ง๐Ÿ‡ชBelgium f0ns

Just checked if it was possible to alter the patch for it to work again.

Looks like it's more work than I anticipated due to https://www.drupal.org/project/simple_sitemap/issues/3484114 ๐Ÿ“Œ Clean up UrlGenerator classes Active

๐Ÿ‡ง๐Ÿ‡ชBelgium f0ns

Patch in #22 was working perfect with 4.2.1 but it doesn't apply anymore on 4.2.2 and the initial problem is introduced again.

๐Ÿ‡ง๐Ÿ‡ชBelgium f0ns

Thanks works great, using the patch in #8 for now.

Small nitpick: I noticed on mobile the 80% width makes small edits, on a phone for example, a bit hard.

I'll try to find a solution for this when I can find the time.

Thanks!

๐Ÿ‡ง๐Ÿ‡ชBelgium f0ns

Thanks works great, using the patch in #8 for now.

๐Ÿ‡ง๐Ÿ‡ชBelgium f0ns

Hello thank you for your feedback, it does indeed steer me in the right direction.

The geocoding might overcomplicate it a bit.

I think I didn't take enough time to try to describe what I was looking for.

In the current module you can drop markers which is great but there is no out of the box way (as I can see) to add some context to a marker (which is shown in the tooltip).

For example it would be great to add markers in a neighbourhood to show locations like:

  • Drop marker on the map and add a label "Post office" (that is shown in a tooltip when viewing the marker on the map)
  • Drop marker on the map and add a label "School" (that is shown in a tooltip when viewing the marker on the map)
  • Drop marker on the map and add a label "Bakery" (that is shown in a tooltip when viewing the marker on the map)
  • Drop marker on the map and add a label "Coffee shop" (that is shown in a tooltip when viewing the marker on the map)

Thank you for the time to answer my questions and the work on this module.

Have a great day

Fons

๐Ÿ‡ง๐Ÿ‡ชBelgium f0ns

Just tested it again and looks good to go.

๐Ÿ‡ง๐Ÿ‡ชBelgium f0ns

Thank you for your answers, I was indeed referring to the widget (and not the formatter) my bad.

As I can see now there is no default way to drop a marker on the map and add some text to the pop-up in the widget.

I guess the only way to get around this for me is to add an address field (that gets geocoded) and a text field field and render that with a view / code on the map myself.

Thank you for your feedback.

๐Ÿ‡ง๐Ÿ‡ชBelgium f0ns

I got rid of the patch and added the preprocess block as suggested in comment #92 and #94.

This works with bigpipe.

Thank you, this is the cleanest solution for now (for me) as I don't have to patch core.

๐Ÿ‡ง๐Ÿ‡ชBelgium f0ns

The patch #28 works for me. I would like to thank you for this work!

A small nitpick, please use - instead of . in patch filenames in the future.

[module-name]-[short-description]-[issue-number]-[comment-number].patch
# e.g. some_module-some-bug-123456-3.patch
๐Ÿ‡ง๐Ÿ‡ชBelgium f0ns

I just tested the patch in comment #4 and everything seems to work fine.

I was wondering if this can get merged in a new release?

For now I'll use the patch as this is one of the modules that holds me from upgrading to Drupal 11.

Thank you!

๐Ÿ‡ง๐Ÿ‡ชBelgium f0ns

Here is a patch that updates the README.

TODO: Update the project page with these instructions.

๐Ÿ‡ง๐Ÿ‡ชBelgium f0ns

Dear gausarts, thank you for the compliment and sorry to read that.

Just a thought:

If we would toggle this setting by default in the install file of the module it would only trigger / set it default for new installs.

Existing installs wouldn't be affected, if you want to affect those (which you don't and I totally get that) you would add this in an update hook.

If you are interested in a patch for the install file i'd be happy to make it but it is not a dealbreaker for me.

Thank you for all your work and have a great evening! There are a lot of good people out there so try to focus on those, that may help!

๐Ÿ‡ง๐Ÿ‡ชBelgium f0ns

Thank you for your reply, I checked the Blazy settings page but it seems that I read over the checkbox to disable these classes.

If I may suggest an upgrade I think it's fair to check this setting by default as you a clean DOM is prefered over all these classes in most cases but that is just my view.

Thanks!

Production build 0.71.5 2024