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

Background image fitting content #472

Closed
wants to merge 6 commits into from
Closed

Background image fitting content #472

wants to merge 6 commits into from

Conversation

dnbute
Copy link

@dnbute dnbute commented Nov 29, 2023

Please always provide the GitHub issue(s) your PR is for, as well as test URLs where your change can be observed (before and after), along with a short summary of changes:

Issue

Fixes #455

Changelog:

  • Used container queries to sort of fix the issue of the image not resizing properly based on content

Test URLs:

Library

  • New Blocks introduced in this PR
    If yes, please provide details below
    Block Name : For e.g. cards

  • Documented in sidekick library

  • New variations introduced in this PR
    Variation Name : For e.g. cards (grid)

  • Documented in sidekick library

  • New mixins introduced in this PR
    Mixin Name : For e.g. compact, white

  • Documented in sidekick library

Copy link

aem-code-sync bot commented Nov 29, 2023

Hello, I'm the AEM Code Sync Bot and I will run some test suites that validate the page speed.
In case there are problems, just click the checkbox below to rerun the respective action.

  • Re-run PSI Checks

@dnbute dnbute requested a review from sdmcraft November 29, 2023 09:52
Copy link

aem-code-sync bot commented Nov 29, 2023

Page Scores Audits Google
/healthy-thinking/soft-picks-advanced1 PERFORMANCE A11Y SEO BEST PRACTICES SI FCP LCP TBT CLS PSI

@dnbute
Copy link
Author

dnbute commented Nov 29, 2023

@sdmcraft This sort of fixes the image not being the right size for its content, but with 2 pretty big caveats

  1. This won't work on firefox since I use :has, I can change some things in the js so I have a specific class I can select, so this is fixable
  2. This is a bit esoteric, I'm not sure how much support there is for container queries (intellisense doesnt even know about them), and I remember I used them at one point the past as well and Dejan got rid of them

@dnbute dnbute marked this pull request as draft November 29, 2023 09:55
Copy link

aem-code-sync bot commented Nov 29, 2023

Page Scores Audits Google
/healthy-thinking/soft-picks-advanced1 PERFORMANCE A11Y SEO BEST PRACTICES SI FCP LCP TBT CLS PSI

@sdmcraft
Copy link

@dnbute , I like the idea of using containers for achieving this. Yes if we can do it without :has , it'd be great. I guess all other features used in here like containers and cqh are widely supported.
/cc @ehrro
[0] - https://caniuse.com/css-container-queries
[1] - https://caniuse.com/css-container-query-units

@ehrro
Copy link

ehrro commented Nov 29, 2023

Hello @dnbute thanks for this actually I wanted to ask for this update.

I agree with @sdmcraft regarding the :has since it's not yet supported by all the browser I would like to avoid using it.

Thanks.

Copy link

aem-code-sync bot commented Nov 29, 2023

Page Scores Audits Google
/healthy-thinking/soft-picks-advanced1 PERFORMANCE A11Y SEO BEST PRACTICES SI FCP LCP TBT CLS PSI

Copy link

aem-code-sync bot commented Nov 29, 2023

Page Scores Audits Google
/healthy-thinking/soft-picks-advanced1 PERFORMANCE A11Y SEO BEST PRACTICES SI FCP LCP TBT CLS PSI

@dnbute
Copy link
Author

dnbute commented Nov 29, 2023

@sdmcraft @ehrro, so, this is sort of the best I could do, I can fine-tune the value some more but if I make it so it works at lower resolutions, it's bad on higher resolutions and vice-versa

Also, this is a kinda weird use-case we have, cause I'm updating the image based on its container, container which has its height set in part by the image, so there's some circularity going on.

Also also this is only for mobile

@dnbute dnbute requested a review from ehrro November 29, 2023 14:07
body > main > div.with-background-image > picture img,
body > main > div.with-background-image > .section-container {
height: 110cqh; /* 110% of the container's height */
@container text-container (max-width: 991px) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By convention, we are following min-width. Could you please adjust for the same? Basically make this the default style. BTW, does it make sense to have this apply to wide screens as well?

@sdmcraft
Copy link

sdmcraft commented Dec 5, 2023

@dnbute , are you planning to merge this? I think this is useful and should be good to merge once you reviewed the comments.
/cc @ehrro

@dnbute
Copy link
Author

dnbute commented Dec 5, 2023

@sdmcraft I'm not sure this actually works for all use cases and I'm afraid it might break more stuff than it fixes; I'd have to test this more before I try to merge it

@dnbute
Copy link
Author

dnbute commented Dec 5, 2023

But something broke on main it seems

Screenshot 2023-12-05 at 10 27 30

@sdmcraft
Copy link

@dnbute , should we bookmark this PR in the linked issue and close this if you aren't planning to take it up anytime soon?

@dnbute
Copy link
Author

dnbute commented Dec 18, 2023

@sdmcraft Yeah sure, unfortunately it seems this doesn't want to behave the way I want it to, and since this is not a high priority change then yeah, we can close it and maybe take it up another time

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.

Soft Picks section for mobile
3 participants