🇩🇪Germany @sascha_meissner

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.

+1 serious problem! patch#9 fixes this for me

The initial patch (removing debounce completely) leads to the dialog not be resizeable anymore itself.. so i now have a new commit which should be way better, e.g adjusting the third parameter of debounce from true to false, seems to work fine.

sascha_meissner → created an issue.

Hey, it´s been some time, i just wanted to add that i totally agree with el7cosmos and edmund.dunn. It is still not the _perfect_ behaviour but at least you (our customers) can use it and work with it (before working with it was impossible)

BTW: I just implented this into the CKEditor5 version.
Dear maintainers, would you rather commit this if there was the same implementation for CKEditor4 version as well? Because IMHO its totally fine to have this feature only in cke5 version, what do you think? It seems a little bit unecessary as i would also have to adjust you provided upgrade path as well and perspectively you should upgrade to5 anyway, what do you think?

Feel free to review the changes. I implemented it so that you can toggle a subtitle per accordion row. The subtitle downcasts to a second

sascha_meissner → created an issue.

+1 Having the same (drastic) issue, patch7 fixes this for me

sascha_meissner → created an issue.

Looking good, i´ll add a small adjustment to the form-description text so the user knows which variables can be used within the callback code

One thought i have is if there should be a default text for the title attribute, maybe it makes sense to use the text-alternative? Ideally this would be configurable, on most of our systems, in example, medias have a title field with a designated title additionally to the text-alternative. This could possibly be a setting within the media tab in text-format edit page?

Hey @smustgrave, thanks for your reply. Surely i will update/adjust the tests for media to also check for the functionality, but i´d say before that the actual code change should be reviewed and in this process probably further developed. It´s the first time I try to contribute something to core, especially using MR´s so please go easy on me :) I actually dont know why the branch is currently not mergeable, i dont know where i find any output about it. But yes i´d love to write the tests as soon as the feature is finalized.

I also added the functionality to drupalImage, for me everything is working pretty good, please review my changes in the 3380223-mediainsert---make branch :-)

Whats still open is that the allowed_html changes by adding the title attribute, i´m not sure how to update this and need help whats the best way to do so. Also, as this functionality is missing in ckeditor5 library it might make sense to port it there.

@wim-leers For me it is working as intended, i didnt experience any sideeffects so far :)

sascha_meissner → created an issue.

Thank you very much @wim-leers for adding some traction to this :)
However we needed to fix this urgently, so i´m sharing this core-patch we use based on Witosos "workaround suggestion" from the github issue, maybe it helps someone who also cant wait until fixed upstream.

Hey, so there is some activity on the jithub issue and they found an error in their schema but also Witoso said that with the Link Plugin it would work, so i dont know if this could also be fixed by utilizing this in core ... and i´m not sure which status to set here, so i´ll set this to "postponed"

FYI: I just created this issue on ckeditor5 jithub

https://github.com/ckeditor/ckeditor5/issues/14683

Hey, i uploaded a video showcasing the bug,

in ckeditor4 when you pressed "return" it would leave the anchor and the style is not apllied anymore, now when pressing return it will keep the style applied but the style itself in the styles-dropdown itself is greyed out, so there is no way to continue writing without the style ... hope i described that clear now :-)

I made some further work and figured out that it is a bug in ckeditor5, i made a custom ckeditor build with the "Styles-Plugin" and have the following code (completely without drupal) with the same behaviour:

<body>
    <div id="editor">

    </div>
  <script src="/ckeditor/ckeditor.js"></script>
  <script>
    ClassicEditor
      .create( document.querySelector( '#editor' ), {
        toolbar: {
            items: [
                'style',
                'link'
            ],
        },
        style: {
            definitions: [
                {
                  name: 'Button',
                  element: 'a',
                  classes: [ 'button' ]
                }
            ]
        }
      } )
      .then( editor => {
        window.editor = editor;
      } )
      .catch( err => {
        console.error( err.stack );
      } );
  </script>
</body>

What would be your preferred way to continue this, would you create an issue on ckeditor on this?

Hey Wim,
Thanks for your reply, unfortunatly I´m having this issue with 10.1.1

we actually also had the issue you linked (styles were not available for <ul>´s ) which was fixed by using drupal core 10.1.1 e.g Ckeditor5 38.
The issue described here is a little different, the "button-style" is available when an anchor is selected, but there is no way to exit it. I´m not sure if this is a bug in the ckeditor library or the "styles-core-implementation", but it should be easy reproduceable like i described.

sascha_meissner → created an issue.

Adjusted text a little bit and added config/install. Feel free to test this and it will be included within the next release

My fellow collegue and I are working on a solution!

FYI: The decision how to go on with this issue is a bit complicated, to keep it short, the current approach to ship translations with this module within config/install/language/... does simpy not work when the default language of your site is not EN, it´s more or less related to this core-issue https://www.drupal.org/project/drupal/issues/2845437 🐛 Process translation config files for custom modules Needs work It also doesnt matter if you have configuration-translation enabled or not, so also when your site just has one language which is not EN, the shipped translations, even though available, will not override correctly.

Therefore my current proposed resolution is to remove the language overrides shipped with the module and let it just contain the english source strings and use localize.drupal to make predefined translations available, just like most contrib modules do. But this not only involves a lot of initial work on localize.drupal but also heavily testing how this affects sites that might already use our shipped translations, it will probably take some time until translations are created/reviewed and available via localize.drupal ...

Thanks Anybody,
so what really needs to be done IMHO is to make it work properly with oliviero and probably also with gin admin theme. Besides that making it a MR seems to be a good step. I hope i can take a dive into this again soon, because on the sites i´m using this it´s actually very handy and on other sites where i dont/cant use this i find myself often in the browsers element inspector and remove the toolbar to see whats under it ... :)

In case anyone might find this useful, this code will remove the edit-langcode element from ContentEntityForms if there are already translations created to prevent the error "Can't change the default langauge"

/**
 * Implements hook_form_alter().
 */
function yourmodule_form_alter(&$form, FormStateInterface $form_state, $form_id) {
  $form_object = $form_state->getFormObject();
  if ($form_object instanceof ContentEntityFormInterface) {
    // Prevent switching languages on already translated content.
    // @see \Drupal\content_translation\ContentTranslationHandler::entityFormAlter.
    $entity = $form_object->getEntity();
    if ($entity->isTranslatable() && count($entity->getTranslationLanguages()) > 1) {
      // Translations exist, prevent language switching.
      $langcode_key = $entity->getEntityType()->getKey('langcode');
      $form[$langcode_key]['widget']['#access'] = FALSE;
    }
  }
}

I´ll take a look into it.