@thomas.frobieter

thomas.frobieter → created an issue.

thomas.frobieter → created an issue. See original summary → .

Yes, +1 for data-attributes!

Right, I built it and its a massive pain. However, as a temporary solution, until https://github.com/vidstack/player/issues/719 is hopefully fixed one day, we build it ourselves to push things forward.

To build successfully the vidstack/player repo needs to be modified:
- package.json: change workspaces "./packages/*" to "packages/*"
- pnpm-workspace.yaml: change 'packages/*' to './packages/*'
- Ensure to install pnpm@8.7.0 globaly using npm
- Ensure to use npm version 18.17.1 using nvm (nvm install v18.17.1)
- In packages/react/package.json replace "vidstack": "workspace:*" with "vidstack": "^1.12.13"
- Afterwards run npm cache clear --force rm -rf node_modules npm install. Next run pnpm install.
- Then you should be able to run npm run build

To be sure, used versions:
- node: 18.17.1
- npm 9.6.7
- pnpm: 8.7.0

Welcome to the node.js building rabbit hole from hell.

Afterwards the required files are located here:
- packages/react/dist-npm/prod/vidstack.js (unminified, I was to able to build the minified version)
- packages/vidstack/styles/player/plyr/theme.css
- packages/vidstack/styles/player/default/layouts/video.css

Maybe we should simply copy the current files from the CDN source instead using this, overcomplicated, bug prone build process:
- vidstack.js: https://cdn.vidstack.io/player
- theme.css: https://cdn.vidstack.io/player/theme.css
- video.css: https://cdn.vidstack.io/player/video.css

Instead of adding the library to the modules repo, we could release it as an NPM package ourselves and use this as a Composer dependency. Seems like a cleaner approach?

I agree with you, we don't need or should wait for the core problem to be fixed, because that could be in 1 week or 10 years.

BUT, we don't use Layout Builder anymore. So we're happy to look at a solution, but we won't be coding it ourselves any time soon.

joachim namyslo → credited thomas.frobieter → .

@grevil you piqued my curiosity - my team still finds this the easiest catch-all, integrated solution for responsive favicons. What are you using now?

Hi there, I am the decision maker on this (same company as @grevil). We still use https://realfavicongenerator.net, but we just put everything directly into the custom theme. That way we can just edit the manifest file, fix the paths... have full control, while not really doing any more work.

thomas.frobieter → created an issue.

@thomas.frobieter, I think you meant the overlay_position fields? They do have a “no overlay” option, which is already present in the default settings form.

Right, this is correct now. One last thing: the animation select now has two "None" options:

<select data-drupal-selector="edit-field-image-animation" aria-describedby="edit-field-image-animation--description" id="edit-field-image-animation" name="field_image_animation" class="form-select form-element form-element--type-select" data-once="field-group-tab-validation field-group-tabs-validation">
            <option value="_none">- Nicht festgelegt/ausgewählt -</option>
                <option value="none">Keine</option>
                <option value="ken-burns">Ken-Burns-Effekt</option>
      </select>

Lets stick with Drupals default "_none" option here. In the version I tested, I only had:

<select data-drupal-selector="edit-field-image-animation" aria-describedby="edit-field-image-animation--description" id="edit-field-image-animation" name="field_image_animation" class="form-select form-element form-element--type-select" data-once="field-group-tab-validation field-group-tabs-validation">
                <option value="ken-burns">Ken-Burns-Effekt</option>
      </select>

1. field_image_animation needs a "none" value (which should be the default in the global settings)
2. The overlay_display fields missing the "no overlay" value

That's it!

@anybody we had considered adding ‘Standard’ to the fields in the media entity, have you discarded this? As I said, it's just a bonus for me. But if it's easy, I'd be happy to do it.

All right.. I forgot that the display settings aren't locked. So this makes totally sense.

However, this is unrelated to the other issues and will take some hours. I won't fix it now.

Btw I think this kind of slang language ("fucks up") doesn't fit here and isn't good for your personal and our company reputation in public. There are similar, better words to choose for an educated individual whatever your mood is. 😉

Not sure whats the problem. Its slang, but it doesn't mean anything bad and nobody can feel offended by it.

Maybe @thomas.frobieter can take a short look at the namings used, if it's all fine?

The names look good (:

I have to admit, I don't care about this typo. Not a bit. The Media slide template is often overridden in our custom themes, I need to scan them all for this.. extremly minor issue.

So we could leave it open, but I don't think I'm going to spend time on it any time soon.

I don't know, if its simply renaming media--drowl-media-types--document.html.twig to media--drowl-media-types--document--default.html.twig .. AND most importantly, not affects oder sites (! Bootstrap), we can do this now.

If it affects older sites, I prefer to close this WONTFIX.

Not sure why we should bloat the templates with this while we have locked the field settings. So.. when someone fucks up the field configuration its PEBKAC?

I think the "default" value will use the value set in the Slick optionset. If this is the case - and I have no idea what else is supposed to happen - we should leave it as it is.

Not a module bug, we set these values custom in our drowl_paragraphs_bs module.

thomas.frobieter → created an issue.

I've tested with 'pointerup' instead 'click touchstart' and it seems to work well on all devices and browsers.

But someone should try this on a real Iphone, I only have the Simulator.

So should we re-open this issue or create a new one?

Thanks @roromedia.

So I changed the task title accordingly.

Just came across this problematic behaviour again on Android. Extremely annoying when simple scrolling triggers the webform dialog.

We should try if a simple cursor: pointer; solves the problem with the click event on Safari, see:
https://stackoverflow.com/questions/24077725/mobile-safari-sometimes-doe...

thomas.frobieter → created an issue.

We switched to DEV and everything seems to work fine!

Drupal 11.1.1 and Office Hours 8.x-1.21

Quickfix:

if(function_exists('office_hours_exceptions_preprocess_field')) {
  office_hours_exceptions_preprocess_field($variables, $hook);
}

However, we don't have the "allow exceptions" option enabled, so .. maybe thats the reason the function is n/a?

thomas.frobieter → created an issue.

thomas.frobieter → created an issue.

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’

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".

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).

text-bg-transparent-dark means "dark" background, so the according subline--light class is correct.

@danchadwick First of all: We don't use Layout Builder anymore in our projects, so we simply accept that its currently broken.

I am not sure about the required custom link attributes, we currently use it only with the Commerce add to cart modal and with Webform like this:

<a href="/webform/feedback" class="webform-dialog" data-once="webform-dialog">
  Feedback
</a>

If I remind correctly, these where the required code changes:

https://gitlab.com/webksde-public/drupal/drowl_base/-/blob/2.x/drowl_bas...
https://gitlab.com/webksde-public/drupal/drowl_base/-/blob/2.x/drowl_bas...
https://gitlab.com/webksde-public/drupal/drowl_base/-/blob/2.x/drowl_bas...

These changes might not be required:
https://gitlab.com/webksde-public/drupal/drowl_base/-/blob/2.x/drowl_bas...
https://gitlab.com/webksde-public/drupal/drowl_base/-/blob/2.x/drowl_bas...

We use https://umap.openstreetmap.de/de/ to integrate very simple maps with just a few custom markers. So the leaflet module is usually overkill.

Here is the Olivero issue: https://www.drupal.org/project/drupal/issues/3497813 ✨ Add further region "Outer Page" to place blocks directly inside the body Active

thomas.frobieter → created an issue.

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.

Done, release incoming.

Merged! thx!

Use the empty value instead of "disabled" string for the disabled option

See 📌 Make media-slide overlay responsive settings mandatory and provide default values Active , so we dont have an empty value anymore after this change.

Add the configuration updates here, or it will never happen, which will screw up existing projects

I don't think we have any configuration updates.

thomas.frobieter → created an issue.

Done. Please review @anybody, I'll create a new release afterwards.

I also create a new release of drowl_header_slides without the media slide form fields states stuff.

I'll create another task for the required configuration update, which is not that important.

The field states are managed in the wrong module (drowl_header_slides), it doesn't matter if the media slide is a "header slide", a hero or a regular slide.

I'll move it over, so we can finish and release this.

@anybody: Okay.. there are still fields that should be hidden by States API, so more explicit.. never hide those three:

- field_overlay_display
- field_overlay_button_color
- field_overlay_button_style

While:
- field-overlay-sizing should be hidden when field-overlay-position is set to "disabled" (or unset) => this is already the case
- field_overlay_sizing_md should be hidden when field_overlay_position_md is set to "disabled" (or unset)
- field_overlay_sizing_lg should be hidden when field_overlay_position_lg is set to "disabled" (or unset)

@anybody: We hide overlay content fields, when "no overlay" is selected (States API). Could you please remove this? It doesnt make sense, since we can disable the overlay for specific breakpoints.

The trigger field is: field_overlay_position, please fix it right here in the issue fork so we dont run into merge conflict ..things.

Not sure how to handle older drowl_base theme versions (Foundation).

As mentioned in the module description, we use Bootstrap classes - old (Foundation based) themes will need to override the affected media templates.

The media templates in 4.x were switched from Foundation to Bootstrap (#3436181: Replace Foundation Twig Templates with Bootstrap Templates) but 4.x will generally also allow to be used with Foundation by overriding and updating the templates.

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?

Okay, so what I have found so far:

- We have z-index bug on Olivero → . Guess this is because i 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 mandandtory, 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)

Sure, that's completely understandable. But it should be made very clear on the module page that this problem exists in Drupal 11, otherwise every new user might spend hours looking for a solution.

A note and possibly a link to this issue is sufficient in my opinion. Users can then decide for themselves if they want to use our Slick fork or create their own.

Maybe just add another point in the "Broken vs. working library versions." Box?

"All versions: Incompatible for Drupal 11+ (using jQuery >= 4.x). You need to use a modified version of the Slick library, see 🐛 Uncaught TypeError: i.type is not a function when using the Slick Media field formatter Active for details."

RTBC for MR!100

thomas.frobieter → created an issue.

As I said in #9, I don't think the Slick module can do anything about this, because it's a Slick library problem.

The maintainers will have to decide how to proceed.

Allright, here we go: https://github.com/kenwheeler/slick/issues/4316

So the Slick library is incompatible with Drupal 11 / core/jquery (4.x).

Feel free to use our fork to have a quickfix: https://github.com/JPustkuchen/slick

But yeah, this is kind of unfixable for this module without a forked Slick version? There was Google dev active in the official Slick repo and merged some MR, but no idea if this will be fixed anytime soon.

I have the feeling this is about jQuery.

The original line throwing the error is:

$.type is not a function

https://api.jquery.com/jQuery.type/ => deprecated since jQuery 3.3, Drupal 11 core/jquery is on jQuery 4.0.

Further testing: It has nothing todo with the field formatter. The same JS error appears when using slick_views.

thomas.frobieter → created an issue.

@anybody could you please review? Its already tested and works well in Safari.

thomas.frobieter → created an issue.

Works like a charm! We can now see the full message. RTBC.

Looks good!

thomas.frobieter → created an issue.

Done https://www.drupal.org/project/photoswipe/releases/5.0.2 →

I can confirm this is because {% apply spaceless %} wraps basically each SDC.

Personaly I use spaceless not very often, because of such problems. I just fix very specific bugs with it.

So from my point of view it should be removed from most (if not all) of the SDCs?

Maybe {{- -}} / {{~ ~}} are better options if trimming is required, guess those have less impact: https://twig.symfony.com/doc/2.x/templates.html#whitespace-control

thomas.frobieter → created an issue.

Definitely, +1 for this.

Yes, please make this feature happen. Adding attributes to link fields is horrible on frontend.

Yep, look right to me.

You are right @mlncn, this was missing from the module description. Thanks! I've added it.

thomas.frobieter → created an issue.

Maybe @roromedia could provide the affected browser? If I remind corretly, this where a Safari bug, not sure if click is still problematic in iOS Safari.

thomas.frobieter → created an issue.

Another example on the user edit form.

When switching to the classes mentioned in #6 'is-invalid' / 'is-valid' on the input elements, it works well (this time on the user edit form).

With 'needs-validation' class on form, without 'is-valid' / 'is-invalid' class on input:

Without 'needs-validation' class on form, but with 'is-valid' / 'is-invalid' class on input:

My conclusion is: In case of the user edit form, the BS client side validation passes, but the server side validation fails. In this case the 'was-validated' class needs to be removed, and the input classes 'is-valid' / 'is-invalid' needs to be set.

Maybe this just needs to be removed from form.theme:

function validateForm(array &$form, FormStateInterface $form_state) {
  if (!empty($form_state->getErrors())) {
    $form['#attributes']['class'][] = 'was-validated';
  }
}

And the 'was-validated' class should only set using JS, like described here:

(() => {
  'use strict'

  // Fetch all the forms we want to apply custom Bootstrap validation styles to
  const forms = document.querySelectorAll('.needs-validation')

  // Loop over them and prevent submission
  Array.from(forms).forEach(form => {
    form.addEventListener('submit', event => {
      if (!form.checkValidity()) {
        event.preventDefault()
        event.stopPropagation()
      }

      form.classList.add('was-validated')
    }, false)
  })
})()

Also note hat .invalid-feedback never shows up without the 'is-valid' / 'is-invalid' classes (my test case on this was, to try to change your user mail address to an mail address which is already assigned to another account => "The email address bar@foo.com is already taken.").

Also on AJAX forms, the 'was-validated' class should be removed after submission:
"To reset the appearance of the form (for instance, in the case of dynamic form submissions using Ajax), remove the .was-validated class from the
again after submission." (See docs).

I now used a dirty workarounded to quickfix this, by removing "needs-validtion" from the form entirely and and set the 'is-invalid' class on the input_classes using:
attributes.hasClass('error') ? 'is-invalid'