Everything looks very good and clean to me now!
Alt+P works fine for me. I've added it to the title text of the toggle buttons!
Additionally, I "think" there is already quite a lot in this MR, so perhaps support for Gin for this feature should be part of a follow-up issue, maybe with a bit more generic approach?
+1 for this.
@steveoriol Did you remove the field wrapper? The module adds the photoswipe-gallery class to the field wrappers attributes. The behaviour of wrapping each image in a photoswipe-gallery wrapper is the fallback if no outer photoswipe-gallery wrapper is present.
Okay, I've added CSS + icons. I've also tested and fixed Gin, with its four toolbar styles.
Moved the .toolbar-expand-floating-button into body, because Gin hides #toolbar-administration in some situations.
I haven't checked smaller viewports / mobile yet.
I think we should not use hide(), show() and toggle() as function names in the JS file, as all three are jQuery functions, which is confusing. My naming is certainly not perfect yet either.
I am not sure about the settings.initial_toolbar_padding_top / settings.initial_toolbar_margin_left. What is the purpose of these settings? I am not a big fan of manipulating styles in JS. If possible, we should set classes instead and put the required CSS in admin_toolbar_toggle.css.
thomas.frobieter → made their first commit to this issue’s fork.
Nice, looks good to me!
thomas.frobieter → created an issue.
@bbruno as lazy-loading is now in core, I think we should instead support that. Could you create a separate issue?
+1 for this. The lazy load module has a lot more options, so maybe someone could implement the support for PSWP as a submodule. But IMO we should only support core lazy loading in the photoswipe module.
Damn, the PSWP version was old for ..reasons. Ran composer update yesterday.. strange. So the feature IS included in the current stable release.
Never mind.. closed.
Oh, I didn't realise that 🐛 Photoswipe formatters are missing image loading setting from core image formatter Active was only a few weeks old. So it just needs a new release?
If this is the case, please close this issue @grevil. Who needs to be triggered to create a new release?
Do you agree to close ✨ Support "Lazy-load" module Closed: won't fix also?
thomas.frobieter → created an issue.
thomas.frobieter → created an issue. See original summary → .
Finally, stylelint is happy.
OK, I've added some fixes, but it's a 'meh' solution. It is very likely that this will require many more fixes for other field types. Gin's help icons are also buggy inside the table. Anyway... an improvement, but far from perfect.
thomas.frobieter → made their first commit to this issue’s fork.
Yes, good idea. I think the core should invent some kind of layout wrapper for the Form API, with attributes for the number of columns, etc. Including the necessary classes + CSS.
Okay done, please review!
The "critical" part is, that we rely on the "fieldset-wrapper" and "fieldset__wrapper" class, which is used by cores Claro admin theme (also by GIN, which is a child theme of Claro): https://github.com/search?q=repo%3Adrupalux%2Fclaro%20fieldset__wrapper&type=code.
However, this wrapper does not have Drupal attributes, so we are unable to add our own reliable class on it. Good enough for me.
Yes, exactly. WE (as DROWL company) never want to have the native controls when using Vidstack.
We could report this to Vidstack - this property name is definitely really, really bad, but I am pretty sure IF this will be solved, it will be in a new major release. So we are blocked on this issue for a long time. So as suggested above, I would prefer to leave the technical field name and just rename the label. Plus add another setting + css code to hide the Vidstack controls. We need to hide the controls for background videos, or for custom implementations where we have our own play/pause button.
Seems to work well in DDEV, so RTBC from my end when @anybody's comments are fixed
I haven't looked at the code, but I think this:
"The fields will be inline by default and responsively become two lines if there's not enough space"
should be very easy to achieve and sounds like the best solution.
@anybody I don't have much hope that this will happen. Twig Tweak typically just passes through to Drupal core functions, as in this case. So if core doesn't offer that, I doubt Chi will agree.
Ah.. session issue on Gitlab, great. Merge train started.
Unable to merge here on Drupal.org or on Gitlab. Buggy hell, let's wait until ... WHATEVER is ready.
thomas.frobieter → created an issue.
thomas.frobieter → created an issue.
Opinions?
51% for:
- Fix the labels from the existing "controls" formatter setting => "Show original player controls"
- Add another setting "Hide Controls" to set the custom class as described in the issue description + add the required CSS code
thomas.frobieter → created an issue.
thomas.frobieter → created an issue.
Well done, everything looks fine now! Thx (:
thomas.frobieter → created an issue.
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>- field_image_animation needs a "none" value (which should be the default in the global settings)
- 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.