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

Make images more responsive #240

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

wimhendrikx
Copy link

More info can be found here: https://www.zachleat.com/web/fluid-images/

Relates to #170 but in this case it also avoids multi-weight selectors by using :where().

Copy link
Collaborator

@jonathantneal jonathantneal left a comment

Choose a reason for hiding this comment

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

Thanks for the great suggestions, @wimhendrikx. I’ve got one request and one question.

assets.css Outdated
Comment on lines 5 to 24
:where(iframe, img, input, video, select, textarea) {
:where(iframe, input, video, select, textarea) {
height: auto;
max-width: 100%;
}

/**
* Do not stretch image or grow bigger than parent
*/

:where(img) {
max-width: 100%;
}

/**
* Preserve aspect ratio
*/

:where(img[width][height]) {
height: auto;
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Would it work to simplify this strategy by changing the initial selector?

Suggested change
:where(iframe, img, input, video, select, textarea) {
:where(iframe, input, video, select, textarea) {
height: auto;
max-width: 100%;
}
/**
* Do not stretch image or grow bigger than parent
*/
:where(img) {
max-width: 100%;
}
/**
* Preserve aspect ratio
*/
:where(img[width][height]) {
height: auto;
}
:where(iframe, img[width][height], input, video, select, textarea) {

Copy link
Author

Choose a reason for hiding this comment

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

Then we miss the img where no width and height is set. We could do this:

:where(iframe, img[width][height], input, video, select, textarea) {
  height: auto;
}

:where(iframe, img, input, video, select, textarea) {
  max-width: 100%;
}

assets.css Outdated
Comment on lines 26 to 34
/**
* Let SVG scale without boundaries
*/

:where(img[src$=".svg"]) {
width: 100%;
height: auto;
max-width: none;
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could we move this bit into a discussion? I’m unsure about it, mainly because .svg seems like such a fragile way to identify the image.

Copy link
Author

Choose a reason for hiding this comment

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

I think if we let this piece out it can cause some strange side affects with SVG images. In the article it doesn't say anything about these lines of code but I think it is part of the whole image solution. Looks like they did put some thought into it.

Copy link

Choose a reason for hiding this comment

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

Width: 100% seems pretty opinionated to me. SVGs are scalable, but they also define their inherent default dimensions the user would then have to revert the rule to.

Copy link
Author

Choose a reason for hiding this comment

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

You are right @kernc. I also did various tests and I think it is pretty save to leave the SVG part out. I will change the PR.

@wimhendrikx wimhendrikx requested review from jonathantneal and kernc and removed request for jonathantneal and kernc November 11, 2022 19:56
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.

3 participants