IMHO ist das Modul selber in allen Sprachen ein "Consent Manager"
Bzgl. der Bennenung der beiden Elemente noch der Hinweis:
Das "notice-element" kann selber auch als Dialog (options.showNoticeAsModal) dargestellt werden, das ist in der Klaro lib selber schon nicht ganz Eindeutig, also bitte nichts mit "-Dialog" ^^
Diese Benennungen werden ja ohnehin nur im administer-Bereich angezeigt, hauptsache die Unterscheidung ist klar, IMHO kann das "Einwilligen/Einwilligung" hier komplett raus, weil worum soll es sonst gehen...?
Ich würde Vorschlagen: "Verwaltungs Element" und "Hinweis Element"
jan kellermann → credited sascha_meissner → .
jan kellermann → credited sascha_meissner → .
works
Reviewed and tested the changes, works good for me, great!
@shortspoken FYI: An unknown source is only logged when the page with it is requested, so you would have to request all pages of your site to find all unknown resources (or let this do a tool like a crawler/spider)
FYI:
The autofocus option is always set to true since 3.0.0-rc7
https://git.drupalcode.org/project/klaro/-/blob/3.x/js/klaro.drupal.js#L28
Had a hard time reproducing the issue and finally found out that it only happens when having twig.config.debug=true and it happens since Drupal added utf-8 icons to the twig debug comments, which must have been a recent update.
both, jan´s and jurgen´s code fix the display of the specialchars in the browser, but also both fail to encode the icons in the twig-debug-comments back correctly :(
I have mixed feelings as the root cause is obviously a bug in the js library as described in the bottom of this issue here https://github.com/klaro-org/klaro-js/issues/503 and we cannot be sure if the area-labelledy will be just ommited when showNoticeTitle=false or if they also go with visually hiding the title when fixing this eventually. So i would propose to support the option showNoticeTitle (not set the default to true though) and the label for the notice title in the settingsform/config, but just remove the area-labelledby attribute in the already existent "accessibilityducktape" in the js-file of this drupal module if showNoticeTitle=false.
This way it would be easier to adopt to a upstream library change imho + we wouldnt have to care of an updatehook and css.
?
Nice to have: The Klaro-Block should be not reusable/only addable once?!
I like the implementation using a block.
I fixed a bug where it would append "null" to the block as well (because #klaro is not existent at that moment yet) + i also added the togglebutton to the block as well and added tabindex=1 to it, so it stays one of the firsts elements to be tabbed to even if the block is placed at the bottom.
Further i experienced several "z-index bugs" depending on which region you add the block, because the css was pretty much expecting the klaro block to be at the end of body, so i´m not sure whether this needs work in the module or if its up to the user then to style that region in their layout?
sascha_meissner → made their first commit to this issue’s fork.
Hey Monaw,
disabling filters in the way the html attribute "disabled" would do for a "vanilla html input" is not supported by this module, but that is due to the fact that exposed filters and their html-structure can also be extended or manipulated in any thinkable way.
Though you can easily hide/remove the filters with this module accordingly so that only 1 of the three filters can be chosen at a time.
Lets assume your "active value" is the string "on" and dff1=compound,dff2=mass,dff3=formula
then, for example, you could use the neq condition (not equals) on the compound admin lable like:
dff2|{neq:on}|AND|dff3|{neq:on}|
This way the compound filter will only be shown if neither mass or formula is selected. You can use the same logic for the mass and formula input as well and achieve your desired behaviour.
PS: Sry for the late reply, i´m on a longer family getaway, hope my answer still helps you
Thank you van.dordafog for your contribution, i´ll have a look into this soon.
Added a little context-filter testing to testStringSearch of LocaleTranslationUiTest
Oops, wanted to rebase current 11.x but messed up, i´ll fix this now
sascha_meissner → made their first commit to this issue’s fork.
@wim-leers
Thank you for also putting efforts into this, it´s been some time and we finally convinced our customer and their seo-agents to drop this feature due to the already mentioned lack of cross-plattform a11y (Even though i personally also like this as a desktop user) IMHO this should be an option, but not provided by the drupal implementation, rather shipped with CKE.
I´ll close this for now
The latest klaro release (rc6) included some improvements related to translations,
could you maybe check if you can reproduce your issue with a fresh install?
Added translation context "klaro" to all translatable strings
Will be included in RC-7!
Will be included in RC-7!
Will be included in RC-7!
Will be included in RC-7!
Will be included in RC-7
Will be included in RC-7
Its an already fixed and duplicate of https://git.drupalcode.org/project/klaro/-/commit/71e920dbfb923cee3008ac...
sascha_meissner → created an issue.
sascha_meissner → created an issue.
sascha_meissner → created an issue.
Fixed a small typo i just stumbled over ;-)
I put some work also regarding this issue into to 1.2.0 release, it contains a general rename of "basefilters" to simply "parameters" and some extra clarity how to use them, enjoy
+1 Pls make a new release :)
Thank you @gapple for your edits and clarify, now it is less confusing ;)
Fixing the JS-Properties in the "adding attributes to script elements" section, as described in my recent comment in the discuss-tab
In the section Adding attributes to script elements
it states
If you want to add attributes to a script tag, you need to add an attributes key to the JSON following the script URL
Where i dont understand why there is JSON , it´s about adding the attributes key to the my_module.libraries.yml, right?
It might be that i am completely missing something here, if not i would make an edit :)
Hey Monaw! First of all , thank you so much for creating the very first issue for this module :-)
I´m more than happy to help you out (hopefully).
So, i see that the term "base filter" is maybe a little misleading, they are only a reference to already existent filters, or better said to request parameter names that this module will "listen" to.
As an example, you have an exposed view filter named "Content type", in it´s settings dialog you see that it´s "filter identifier" is type (Like in your screenshot). Then you can use this module to display other filters or fields based on the value of the "Content type" Filter. Therefore you would need to let this module know that it should listen to the parameter "type" and its value... So you would set "dff1" to "type" .
So unfortunatly i´m not sure what you are trying to achieve, but now that dff1 is "listening" to the Content Type filter you can use it in the administrative title of a field or filter to dynamically show/hide them based on the value of the Content Type Filter.
In example, you have another exposed filter "Author name" that you only want to show if the selected Content Type is "article", then you can write in the administrative title of the "Author name" filter dff1|article| , speaking: "Show this filter if the value of dff1(type) is "article".
Please let me know if this clarifies the usuage of this module for you :) If not, maybe explain more in detail what you want to achieve. Additionaly i will try to work on the README and add some examples somehow
sascha_meissner → created an issue.
I just added translation context "klaro" to _all_ translated string, not quite sure yet how this affects already existent translations
Done, will be part of the next release
sascha_meissner → created an issue.
sascha_meissner → created an issue.
Hey juagarc4,
thanks for your update! i added hook_library_info_alter that grants compatibility for both installations, e.g it will work if installed in libraries/klaro or libraries/klaro-js, also added a watchdog message if none of them are found. + i updated the contents of composer.libraries.json with the new package so there is also backwards compatibility if anyone already included the composer.libraries.json in their root composer.json
i accidently pushed (also your commits) to 3.x of origin, but i´d say its a happy accident because i wanted to keep your changes anyway ;)
So i want to update the Readme a little bit when there is an opportunity and test a little bit and then it should be part of the next release :-)
@juagarc4 Thank you for your input and your suggestions, i´d say it would make perfectly sense to adapt to the latest guide on "managing dependencies for a custom project" i wasnt up to date on this development, we just added the merge-plugin approach for convenience, if i remeber correctly we did so because webform plugin did it like this. And yes this requires to manually include the composer.libraries.json into your root composer.json TBH in our own projects we use a custom scaffold that will only put the two required dist/js and css files into libraries/klaro because we think its not the best practice to have the "whole library" inlcuding example pages and so on publicly within libraries folder when you just need the built files, this probably led to the decision not to force anyone in using a specific way on how to put the files into where they are expected. I will continue on this tomorrow and check with my collegues what they think!
Hey juagarc4,
thank you so much for reporting and working on this issue!
I looked into this and actually it looks like different repository than suggested in the readme of this module.
This module is shipped with a composer.libraries.json which will contain the expected library
e.g.:
```
"kiprotect.klaro": {
"type": "package",
"package": {
"name": "kiprotect/klaro",
"type": "drupal-library",
"version": "0.7.18",
"dist": {
"url": "https://github.com/kiprotect/klaro/archive/v0.7.18.zip",
"type": "zip"
}
}
}
```
But it seems that klaro has changed it´s github structure and also the library name, still the shipped snippet above still is working.
I will checkout how to continue on this, but if changing the library path would be the solution one would also have to think about all existent installations where the library will not be able to be loaded anymore because the path is wrong for them :/
@DamienMcKenna Thank you for the quick responses! I applied the "code-simplifications" from the other issue here and also added them to the first call in the same way
DamienMcKenna → credited sascha_meissner → .
sascha_meissner → created an issue.
@handkerchief Works fine now with gin as admin-theme as well :-)
Built in a little check if it´s gin and if so also hiding the "gin-secondary-toolbar--frontend" and correctly overwriting the paddings, this didnt work before because gin uses !important rules here, please review and test.
PS: I like the idea of the variant-plugin-system introduced here,
but there needs to be some further work done to be able to actually use it.
Right now it seems a little bit pointless because the WAI-ARIA-Patterns-And-Widgets library will always get loaded and has checks within itself for the element type, e.g "panel must be div", "heading must be H1-H6" so there is no point in making the element-types configurable when the accordion will not initialize then. In example, a customer of us dont want to use H-elements for the title because the headline-hierarchy differs between pages or places in pages where you can embed an accordion.
I´m thinking of extending the VariantPlugin with associated libraries that will be loaded according to the chosen variant.
Also it would be cool if the WAI-ARIA-Patterns-And-Widgets library would have an option to initialize more than just one row opened.
Did some further a11y testing with browser tools and WAVE, seems to work good, tried using screenreader "pericles" firefox extension which did not read the labels, only the contents but i dont know if thats a referrence.
Personally i can just add that the focus styles are barely visible, so its hard to see where you are when using keyboard navigation, which is not a big problem for me because i´m heavily overwriting the css anyway.
So I just realized that the release and tag 2.1.0 was made from the 2.0.x branch and is not reflected within 2.x (which would make sense afais) So right now the issue fork should have the state of latest release tag 2.1.0 + the rerolled and applied patch #32
Unfortunatly when trying it out it seem to not fully work for me. I followed the steps in updated readme.
The new libraries css and js files are getting loaded in frontend, i can also choose default variant in ckeditor_accordion settingsform, and add the accordion button to the toolbar in text format without problems, i can create accordions in ckeditor with no problems, but the Filter that should replace the markup is not called at all, i just get the dl, dd, dt markup rendered in frontend. Am I missing something here? Does this have to do with the upstream changes to 2.1.0 ? I try to further investigate ...
Hey, +1 for the issue and the work already done, to get some traction into this i just rebased origin/2.x into the issue fork, applied patch#32 on it (Had to reroll it because it didnt apply to CkeditorAccordionSettingsForm on tip of 2.x) and pushed into issue-fork, see https://git.drupalcode.org/issue/ckeditor_accordion-3124167/-/tree/31241... . Now we have the development in git i will start to review the work already done.
sascha_meissner → made their first commit to this issue’s fork.
@handkerchief, now I see what you mean... it works good for the "regular" admin-toolbar in bartik or oliviero, but it has issues on gin-theme i didnt test. Let me see if i can figure something out for gin
It´s been some time ... just rebased origin/10.1.x into my issue fork (which was a little nasty because i needed to rebuild ckeditor5 for every commit :P ) Also didnt manage yet to join the slack or do any other progress related to the topic, at least you can patch 10.1.8 now with the diff
Hey i just implemented what I was describing in my last comment.
You can now use the key-combination "Shift + H" to toggle the toolbar, IMHO this is working veeeery good, i like it very much, also my team does :P
Due to the fact that there is no css required anymore it just works, no matter if you use Claro or Oliviero or other theme. I also removed the differenciation between admin and non admin routes, because i didnt see the point anymore.
Also i created an issue fork and a mergerequest.
Please test and review my changes :)
Thank you for the feedback :-)
This was my first try to contribute something when i started learning Drupal, it turned out to be more complicated than i thought due to the fact that admin-themes are variable and come with their own markup. Now two years later i´m thinking that it would still be a nice feature, gin-theme tried to adress it a little bit by putting stuff into the sidebar but still there is no way to also remove the top-bar. So what i am thinking of right now is that the toggling could be made with a keyboard-combination sth. like "shift + h" with no button at all, so there the whole problem-layer with positioning and styling the button would disappear. I´ll try to code something up when i have an oppurtunity/time :)
Thanks to @bwong for the solution :)
Looking at the Readme->Configuration->Nr.5 where it states "If none are selected all formats will be allowed" seems to be incompatible with Readme->Configuration->Nr.8 "Ensure that the 'path' for the data export is a file, with an extension
matching the format. ie: /export/foo.csv"
I just tested 10.2.x-dev and it is working/fixed as expected. Thank you very much 🙏
merged and relased with 3.0.0-rc6
merged and released with 3.0.0-rc6
Released with 3.0.0-rc6
Merged and released with 3.0.0-rc6
Done, merged and released with 3.0.0-rc6
sascha_meissner → created an issue.
This will be part of the next release RC-6
sascha_meissner → created an issue.
Thank you for reporting this, i debugged this a bit and came to the conclusion that the klaro library expects to be loaded in a single js file, orherwise (bundled with other scripts using drupal aggregation) it will struggle to determine the currentscript the library was loaded with.
This needs to be fixed upstream, i created a new issue for this as the mentioned issue only applies to IE11, I can reproduce the error in any browser i tested though.
https://github.com/klaro-org/privacy-manager/issues/488
Until then i guess it makes sense to use the workaround to disable preprocessing
Hey Van.dordafog thank you for your support!
This might be related to
https://www.drupal.org/project/klaro/issues/3161254
🐛
Text translations not working properly
Active
is BE also the default language on your site?
I´m going to test it though, i´m not sure if renaming app to service is sufficient fix to merge
Thank you very much, merged into 3.x and part of next RC!
sascha_meissner → made their first commit to this issue’s fork.
@smustgrave Do you mean a functional javascript test to test for the wrong positioning like in the video I uploaded? Would you want this to be integrated this into any of the already existing test-classes? I´m actually not sure why the LayoutBuilderDisableInteractionsTest::testFormsLinksDisabled did fail, nor do I know how to rerun the test without making a new commit.