There are multiple problems here.
1. First problem is with selector:
$('#drupal-off-canvas-wrapper, #drupal-off-canvas').dialog('close');
That comma means that this will return two jquery elements, one for #drupal-off-canvas-wrapper and the second for #drupal-off-canvas and then dialog('close') will be called for both elements. However, #drupal-off-canvas-wrapper does not have dialog object attached to it, it is a wrapper for #drupal-off-canvas which does have dialog object. Calling dialog() on #drupal-off-canvas-wrapper will produce error from issue description:
Uncaught Error: cannot call methods on dialog prior to initialization; attempted to call method 'close'
This selector change is first proposed in MR !13 in comment #3448524-30: Automated Drupal 11 compatibility fixes → , re-mentioned in #3448524-33: Automated Drupal 11 compatibility fixes → and then introduced in commit 1866c00f. This is done as explained in previous comments in order to keep D9.5 support. However, proposed change should be done to keep D9 CSS compatibility as explained per change record https://www.drupal.org/node/3305664 → - this is just related to CSS selector that needs to be changed but not JS selectors. I guess somebody misunderstood this change record and though that we also need to change JS selector, which is not true. The selector for dialog element is the same, and it is `#drupal-off-canvas`.
Now, MR in previous comment #1 is proposing to switch to
$('#drupal-off-canvas-wrapper .ui-dialog-titlebar-close').click();
which is fine, but it is perfectly OK and a bit faster to just revert to previous code that was working before which is
$('#drupal-off-canvas').dialog('close');
2. Second problem is a data attribute that is never defined:
$('.toolbar-icon-moderation-sidebar').on('click', function (e, data) {
if ($('.moderation-sidebar-container').length && (!data || !data.reload)) {
According to documentation data attribute is used to pass additional data to event.data property when event is triggered. However, `data` variable is never defined in the first place, and it looks to me, it should be removed. Maybe this was used in the past, and then got refactored but this code was not removed?
Note that this data attribute does not produce any bug, but is not doing anything usefull here and can be removed.
3. Finally, when viewing a node and with opened moderation sidebar if you click again on top right `a.toolbar-icon-moderation-sidebar` link first event that will be triggered is ajax event for /moderation-sidebar/node/nid/latest callback which will trigger loading of the sidebar again. Then event from moderation_sidebar.js will be triggered for closing this sidebar. This will result in first closing the moderation sidebar but then opening it again when ajax callback is finished - meaning sidebar will just reopen which is not expected behavior.
There are multiple ways to solve this and put moderation_sidebar event to be triggered first, before ajax event handler. Seems to me that using addEventListener() useCapture parameter is the easiest way to do it.
New commit in MR is addressing all three mentioned issues.
pivica → made their first commit to this issue’s fork.
Merged.
MR is ready. There is one additional small change
- if (preg_match_all('/\s\*\s(.*?)\n\s\*\/\nfunction\s' . $parent_theme . '_bs_update_(\d{4})/', $content, $matches)) {
+ if (preg_match_all('/\s\*\s(.*?)\n\s\*\/\nfunction\s' . $parent_theme . '_bs_update_(\d+)/', $content, $matches)) {
That will allow writting of update function with more the 4 numbers like
function bs_base_bs_update_200001($target_theme_name) {}
> In the documentation you linked it is suggested that the role and aria-label attributes should actually be on the
(or other container) element, not on the input field. Is there a particular reason why you placed it on the input field instead?
Testing this on a client website, Chrome is happy with this MR and adding aria-label to form tag, it does not report a11y error any more about missing label on input field. However when testing with Wave Evaluation Tool it is still reporting error that label is missing for the input field:
Reading above referenced mdn documentation it says that search landmarks should only be labeled if there is more then one search on the page, and if not then you do not need to provide a label:
If there is more than one search landmark role in a document, provide a label for each landmark.
But Chrome Lighhouse will flag a11y error if form only have a search role without a label on form or input element.
Clearly a11y tests and specification are a bit off here and somebody is not doing things correctly. But what should be correct solution here i am not sure for now.
One more potential problem, what would happen in a rare case when you have two search text fields in a search form for some reason - then defining a lable for both text fields would not work properly.
IMO adding aria-label definition on input field and then using in on form tag feels wrong to me. How about adding two configurations - one for form tag and then other for input text elements? With this we would cover all cases, right?
It feels to me that form aria-label config should actually go to a views module it self, because any exposed form is a in a way is a search form and it should have a search role and aria-lable describing in more detail what that form is searching.
This change broke aria-labelledby a bit. But we can also simplify its value now and just use parent_id var which we added in previous commit. True we will not reference all the parents of menu item, but just the first one, but I think there is not too much benefit of referencing them all in the first place, lets see. This is fixed in https://git.drupalcode.org/project/bs_base/-/merge_requests/17/diffs?com....
MR ready for testing.
Makes total sense. Thx for your support :)
Hi, thx for merging this. I've released umami_analytics 2.0.0-beta2 which is now compatible with Klaro support.
> Small notice: you may enable "optOut" it the tool is really GDPR-compliant (but optOut is not working because of a bug in upsream klaro-js).
Yeah it should be GDPR compliant because it does not use cookies nor stores ip addresses. Thanks for the optOut tip, shoudl we then change service configuration for Umami to be optOut by default or we are leaving site admins to decide per use case?
Ready for review. Note that I've changed html head id to umami_analytics_tracking_script in latest dev, will create a new release if this gets into Klaro.
@drumm thx for your feedback, I will organize 2.x development then how you suggested.
BTW exactly the same error I made in bs_base → contrib theme on https://www.drupal.org/node/2839205/edit/releases → - I can't mark 2.x-dev as a Supported version because that checkbox is disabled.
Discussed with @berdir and he proposed that we merge this now and create a new release so we can start internally testing Drupal 11.
As explained in #3491751-5: Drupal 11 compatibility → you do not want to use bs_base or bs_lib with Drupal 11 for now, but if for some reason you still want to do it then you need to patch Bootstrap util.js to allow it to work with Drupal 11 jQuery 11 version.
Merged.
Merged.
Merged, and as @berdir noted for this to really work you need to patch manually Bootstrap 4 util.js to be compatible with Drupal 11 jQuery 4 version, something like:
- var maxMajor = 4;
+ var maxMajor = 5;
However you should probably not do this, we need this internally so we can more easily start working on the Bootstrap 5 port and test stuff.
Merged.
Merged.
So Bootstrap 6 dev plan is out (https://github.com/twbs/bootstrap/pull/41236) and the plan is that Bootstrap 6 will switch to Dart SASS which probably means we can't switch for now and we will do this when we start porting to Bootstrap 6.
MR ready for testing.
pivica → created an issue.
First MR ready, took a shortcut:
// TODO - return displays of only target paragraph bundles.
// For now just return all paragraph displays until we improve this.
if ($field_definition instanceof FieldConfig && $field_definition->getType() == 'entity_reference_revisions') {
$displays = $this->entityDisplayRepository->getViewModeOptions('paragraph');
break;
}
This is more then enough for now to test this. We can improve it later ;) And seems it is working fine.
Ready for a review.
Ready for a review/test.
@dhruv.mittal thx for trying to contribute to this issue, but i am afraid this is much more complicated then just changing package in package.json and in related gulp JS files.
At a very minimum we need:
- All SASS code needs to be updated and deprecated SASS code fixed. Check related Bootstrap 5 issue for more details on this https://github.com/twbs/bootstrap/pull/41112
- We need theme update function that will update child themes automatically - similar what you did in MR
- A lot of testing and maybe couple of more things we will for sure discover along the way ;)
Merged.
Merged.
MR is ready for a check, and a fix is quite simple because I just optimized initialization and usage of scrollTarget and just made sure it is always a jQuery object no matter of code path execution.
> Is this something that can or should be fixed in Drupal Core or is it more of an Entity Browser bug that we should move to that project?
@cilefen I think the actual problem is in Drupal.AjaxCommands.scrollTop which does not properly initialize scrollTarget variable for all edge cases.
Merged, 2.0.0-beta1 release created.
This can be closed in favor of 📌 Drupal 11 compatibility fixes Active .
> Are we going to do a 2.0.0 release?
All the same to me.
If we want to add some more features from project page todo list to 2.x before first stable release then it make sense to do beta release now. If not then we can do 2.0.0 release now.
I do not have time in next months to work on this module, so from my side new features will be added only when I need them in some client project.
@jackwrfuller what would you prefer, 2.0.0 stable release or some 2.x-beta1 release for now?
@jackwrfuller did some additional minor depreciation warning fixes and I've prepared 2.x branch. Do you have time to quickly test latest MR version before we merge this?
Thx for pushing this implementation.
I didn't test anything because I didn't use https://www.drupal.org/project/cookies → contrib module for now so not sure how all this works.
Did a quick review of the MR and I think the name of the submodule should be microsoft_clarity_cookies and not cookies_microsoft_clarity. Can we rename this string in all places?
Provided couple of additional minor feedbacks in MR.
After this is done all I can do it, to test this, is to enable modules locally and check is there some exception, I can not test actual integration. If somebody else can test this and verify that it works that would be great. If not we can merge this and then check future issues in follow-ups.
@jackwrfuller thx for doing an effort to port this to D11, great stuff.
I've provided some feedback in MR.
What do you think, should we do this in 2.x branch so we keep 1.x branch compatible with D9?
Fixed. I've also used new CSS vars for this instead of old SASS vars.
rc1 release is done.
Done in https://www.drupal.org/node/3487223 → . I will add more nice examples over time, feel free to add examples you built in the past.
This is ready for a full review/test. One thing that needs additional thinking is handling of text like links - this are the links that starts and ends with text and that can have simple html elements between text. Simple html elements are elements that do not have additional child elements inside. The idea is that wrapping should work for links like
<a>link with just some text</a>
<a>link with some <strong>bold</strong> formatting</a>
But complex links like
<a><div class="card"><div class="title">link card title</div><div class="body">link card</div></div></a>
will not be wrapped.
The questions are edge cases like
<a><span>link</span> with just some text</a>
<a>link with just some <span>text</span></a>
Is this a text like link or not, current assumption is that it is not because there are elements on start or end.
Maybe we should just say, link is text like if there is no elements inside of it or it has only elements that have text and no additional inner elements?