Added 'Refactor Sass Configuration in Gulp Tasks' secion.
I've decided to remove suffix `-label` from id - it does not add any value to the id itself.
The rest is good now, merged.
Thx for your work anirudhsingh19.
> I have also encountered that only level 1 of menu is getting rendered.
In order to render child levels, parent menu items needs to be expanded, so you need to enable 'Show as expanded' option for parent menus.
I've reviewed MR and I think we do not need a second `clean_unique_id` call for aria_id.
@anirudhsingh19 thx for trying to work on this.
Note that bs_bootstrap subtheme is deprecated in 2.0.x dev version and most code from it is removed or moved to parent bs_base theme. The template that needs fixing is now located in templates/navigation/menu--navbar.html.twig.
Explaining @font-face supported properties in 8.x-1.x and 2.0.x versions.
This conflicted with latest 2.0.x version. Fixed and also switched from var to const for variables.
Done.
MR ready for a review and testing in actual projects.
Done.
Done.
Merged.
@berdir suggested adding BROWSERSLIST_IGNORE_OLD_DATA into gulp task so we do not need to do it in ddev or in some other way. Now we suppress caniuse-lite old db warning always.
Updates related to new icons font cache bust hash calculation.
Update documentation related to new icons font hash calculation.
@berdir provided couple of feedbacks, all are addressed, this is merged.
MR is ready for a review, it is in the draft mode only because first we need to commit ✨ Automatically add cache bust query string to icons.woff Active because this MR is build on top of MR from that issue.
MR ready for a review.
The whole thing is fit into the current theme build process, so nothing is changed there. The step package.json `build-icons` step now use execute `node build-icons.js` and all hash calculation and update of needed files is done there. Hash is injected as a SASS variable - with existing $bs-icon-font-hash variable, and it is also injected into theme info yaml file if icons font asset preloading is used.
I considered couple of approaches for this but on the end I sticked to this solution because this is how we currently update are icons assets - child themes icons and build process is separated from parent theme icons and build process.
New update function `bs_base_bs_update_200003()` should update child themes to this new hash fully so no manual update process is needed.
Merged.
MR is ready for a review.
Patch is updated against 📌 Deprecate bs_bootstrap subtheme Active merge request. Moved this to bs_base, added missing jQuery dependency in library definition and removed Drupal var from JS code.
Keeping this still in patch for now and when 3474789 gets in we could create MR if needed.
This is in progress now, above meta issue description will need update, however all of above questions and consernes are obsolete now and not relevant any more.
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.
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.