Review COOKiES markup / CSS before 2.0.0

Okay, so what I have found so far:

- We have z-index bug on Olivero. Guess this is because I've placed the Cookies UI block in the footer region, which is set to z-index: 1 by Olivero. So this is probably more a documentation, not a code thing => "Place the COOKIES Ui block outside the regular page structure (directly inside the body element is recommended)". However, Olivero does not provide such a region, so I am not sure if this requires further work on the cookies side, as Olivero is the default Drupal theme.
- The BEM classnames are incorrect, if the library is ment to fit the Drupal standards this should be fixed? Example: We have cookiesjsr-banner and an 'active' class on the same wrapper, the active class should be cookiesjsr-banner--active instead (Formatter class). Another example are the child elements of cookiesjsr-banner, currently we have cookiesjsr-banner--info, this shouldn't be a formatter class, it should be a child element class: cookiesjsr-banner__info, etc.
- cookiesjsr-layer--overlay is a button element, which is quite unusual. I'd recommend to change it to a simple div. There is a high risk that the button element will recive unwanted stylings in some themes.
- .cookiesjsr-layer: This change is not mandatory, but would improve the stability of the layers layout. Instead using padding to reserve space for the header and footer, the layer should use display: flex; flex-direction: column; ... so there is also no need to set a fixed height to the header and footer (=> flex: 0 0 auto; and flex: 1 1 auto on the .cookiesjsr-layer--body)

I'd like to hear if @JFeltkamp agrees to this points before investing further time. I guess all the points (except the block/region issue) are related to the library, not the Drupal module?

Hey Thomas, thank you for the review.

Here my thoughts about your points in detail.

1. Z-index problem Olivero Theme. Indeed we should provide a stable and good solution. But we should not modify the Olivero theme in our CSS. I think we should report an issue for Olivero and add a comment in our description section: E.g.

Known issues:
OLIVERO Theme - To avoid known display (z-index) problems, insert the COOKiES UI block at the end of the "Header" section.

2. BEM class name - This is not a big win to fix it. After the fix the CSS and JS has to be updated in sync. May be we have cross over issues. I would have done nothing here.

3. Button element: This is not a bug. Due to accessablity all clickable elements must be declared as button (or A-Tag ore needs a tabindex and aria-label). The background should be clickable to get rid of the Dialog.

4. CookiesJSR Layer Flexbox: This could be a good improvement.

That's true and I'm fine with it.

So if I shall do that, please give me some more detailed instructions. I'm not that familiar with these best practices.
If Thomas wants to do it, I will make the review and give some support for the changes in Svelte.

Yes, I think it will be easier if I fix the class names directly.

I create a fork of: https://github.com/jfeltkamp/cookiesjsr/tree/2.x/src

I'll try to solve it within the next two weeks!

1. Z-index problem Olivero Theme. Indeed we should provide a stable and good solution. But we should not modify the Olivero theme in our CSS. I think we should report an issue for Olivero and add a comment in our description section: E.g.

Known issues:
OLIVERO Theme - To avoid known display (z-index) problems, insert the COOKiES UI block at the end of the "Header" section.

We might should suggest a new region to Olivero "Outer Page" or something like this, located as a region at the bottom of the body-tag.

4. CookiesJSR Layer Flexbox: This could be a good improvement.

I'll try this out in in the library fork too.

Here is the Olivero issue: https://www.drupal.org/project/drupal/issues/3497813

FYI according to the tests, the "bannerText" should be inside a span with the class "cookiesjsr-banner--text":

    $session->elementTextEquals('css', '#cookiesjsr > div.cookiesjsr--app > div.cookiesjsr-banner > div.cookiesjsr-banner--info > span.cookiesjsr-banner--text', $cookiesTexts->get('bannerText'));

But currently, the text sits directly inside "div.cookiesjsr-banner--info". Just a heads-up.

Done @jfeltkamp, please review: https://github.com/jfeltkamp/cookiesjsr/pull/42

BTW: Very well written, who actually needs Klaro :P

FYI according to the tests, the "bannerText" should be inside a span with the class "cookiesjsr-banner--text"

Guess the tests needs to be adjusted, I am pretty sure the class was renamed to 'cookiesjsr-banner__info' ('cookiesjsr-banner--info' before my changes). (the info class is also present in the test selector, so my assumtion was wrong)

I don't currently have the time to delve into this topic (nor am I really interested). So if this needs to be fixed anytime soon, someone else should do this.

The class "cookiesjsr-banner--text" is not present in the new library, so its most likely "#cookiesjsr > div.cookiesjsr--app > div.cookiesjsr-banner > div.cookiesjsr-banner--info".

Ok, so we don't need the span anymore? Then I'll just adjust the tests accordingly and that's it :)

I can't tell what it used to be good for, but it's out and Joachim will scream if something doesn't fit ;)
So: ‘#cookiesjsr > div.cookiesjsr--app > div.cookiesjsr-banner > div.cookiesjsr-banner--info’

Made a review of Thomas' changes.
Everything still works fine, also the improvements in header and footer, what I like.

Created new version: 2.0.5

These guys using 1.x with extended custom changes will not be amused - But our motto is: Now or never! - So now :-)

@jfeltkamp We recently switched from Klaro back to Cookies and as I was about to make some minor adjustments to the design, I noticed that the old class names are still present in 2.0.0-alpha4, while it looks correct in the svelte source file (TheBanner.svelte).

Any ideas?

vs.