Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Tweak: Add caption space option to image gallery and image carousel widgets [ED-10064] #20727

Merged
merged 32 commits into from
Dec 25, 2023

Conversation

rodolphebertozzo
Copy link
Contributor

No description provided.

@rodolphebertozzo rodolphebertozzo changed the base branch from master to release/3.10.0 December 13, 2022 14:08
@rodolphebertozzo rodolphebertozzo changed the title Tweak: Add caption space option to image carousel widget Tweak: Add caption space option to image gallery widget Dec 13, 2022
kobizz
kobizz previously requested changes Dec 15, 2022
includes/widgets/image-gallery.php Outdated Show resolved Hide resolved
@rodolphebertozzo rodolphebertozzo changed the title Tweak: Add caption space option to image gallery widget Tweak: Add caption space option to image gallery and image carousel widget Dec 21, 2022
@rodolphebertozzo rodolphebertozzo changed the base branch from release/3.10.0 to release/3.11.0 December 28, 2022 18:19
@rami-elementor
Copy link
Member

I personally like this PR, it adds consistency to Elementor widgets using image captions.

It's in my TODO list to test this patch.

@rami-elementor rami-elementor changed the base branch from release/3.12.0 to release/3.13.0 March 10, 2023 09:25
@rodolphebertozzo
Copy link
Contributor Author

@doronwolf Hi, I've corrected the underlined aspects by adding conditions and default values (0.75 rem = 12px) so that there won't be any backward compatibility problems with sites that already display captions. Adding this control without a default value surely overrides what is agreed in the SASS file at this level.
As for the lightbox, it's very strange. Any ideas?

@doronwolf
Copy link

Hi @rodolphebertozzo thank you very much for making the necessary changes 🙏 looks very good!
about the lightbox - I agree, it seems not related to your code
I need to do a few more tests on my end but I believe they will go smoothly and then we all approve and merge this PR
will try to update here once I have news

@rodolphebertozzo
Copy link
Contributor Author

rodolphebertozzo commented Jul 11, 2023

Hi @rodolphebertozzo thank you very much for making the necessary changes 🙏 looks very good! about the lightbox - I agree, it seems not related to your code I need to do a few more tests on my end but I believe they will go smoothly and then we all approve and merge this PR will try to update here once I have news

Hello,
I've just checked the SASS files, it seems that there are no default style rules for the caption, In my opinion, adding this control automatically imports certain default values contained in other files, which has an impact on existing sites. That said, by specifying the value ourselves from the control, we get around the problème. Yes
, I've just restored all the files as they were, and the problem is not with my PR

@doronwolf
Copy link

Hi @rodolphebertozzo I also checked again... there is a default style rules for the caption - but it comes from the theme (see the attached screenshot) in my case it's the 'hello' theme
Screenshot 2023-07-12 at 9 46 48 but since we can't know which theme each user chooses to use, and other themes may have different default (or no default) then I agree with you👍 and thanks for the catch

I hope this is the only change you reverted? because initially when choosing the 'hide' option for the caption, the spacing slider, and the text-shadow were still available, and that was a fix you shouldn't revert

@rodolphebertozzo
Copy link
Contributor Author

rodolphebertozzo commented Jul 12, 2023

Salut@rodolphebertozzoJ'ai également vérifié à nouveau... il y a des règles de style par défaut pour la légende - mais cela vient du thème (voir la capture d'écran ci-jointe) dans mon cas c'est le thème 'hello' mais puisque nous ne pouvons pas savoir quel thème chaque utilisateur Capture d'écran 2023-07-12 à 9 46 48choisit à utiliser, et d'autres thèmes peuvent avoir des valeurs par défaut différentes (ou aucune valeur par défaut), alors je suis d'accord avec vous👍et merci pour la prise

J'espère que c'est le seul changement que vous avez annulé? parce qu'au départ, lors du choix de l'option 'masquer' pour la légende, le curseur d'espacement et l'ombre du texte étaient toujours disponibles, et c'était une solution que vous ne devriez pas revenir

For text shadow and spacing, I've added the condition that the control is only displayed if the caption is active, or should I come back to it, which would be strange

includes/widgets/image-carousel.php Outdated Show resolved Hide resolved
includes/widgets/image-gallery.php Outdated Show resolved Hide resolved
includes/widgets/image-gallery.php Outdated Show resolved Hide resolved
includes/widgets/image-carousel.php Outdated Show resolved Hide resolved
includes/widgets/image-carousel.php Outdated Show resolved Hide resolved
includes/widgets/image-gallery.php Outdated Show resolved Hide resolved
@rami-elementor rami-elementor changed the title Tweak: Add caption space option to image gallery and image carousel widget [ED-10064] Tweak: Add caption space option to image gallery and image carousel widgets [ED-10064] Dec 24, 2023
includes/widgets/image-carousel.php Outdated Show resolved Hide resolved
includes/widgets/image-gallery.php Outdated Show resolved Hide resolved
includes/widgets/image-carousel.php Outdated Show resolved Hide resolved
includes/widgets/image-gallery.php Outdated Show resolved Hide resolved
rami-elementor
rami-elementor previously approved these changes Dec 24, 2023
@rodolphebertozzo
Copy link
Contributor Author

Hello @rami-elementor,
Yes, I've just been made aware of your changes. At the time I started working on this improvement, I wasn't fully aware of how Elementor works with units, and even less of the fact that they are set in a separate file, and that specifying maximum, minimum or default unit values is only done if you want to get away from the operating logic in a specific case.

@rami-elementor
Copy link
Member

@rodolphebertozzo

Thank you for your valuable contribution! I approved this PR. Moving to @arielk to merge.

arielk
arielk previously approved these changes Dec 25, 2023
@rami-elementor rami-elementor dismissed stale reviews from arielk and themself via 0a1a32d December 25, 2023 13:00
@arielk arielk merged commit d4ea72f into elementor:main Dec 25, 2023
49 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants