[img-helpers] Add/Edit Image display utilities #357

stacyk · 2023-05-17T18:22:16Z

Description

Add/edit image helpers:

Fix img-shadow (and convert all ing-border to use ing shadow instead)
Add ability to display an image to the left of the text on large enough screens (media-pull)
Remove extend-small (it was the same as what the figure width is)
Add caption padding for images that use extend-full
Add examples to the Sample Pages / Placing Images page
Remove unused sizes and extend-left extend-right classes
Update some of the older embed.img macros to the embed.figure macro (especially the ones that were wrapped in a <figure> tag

Steps to test/reproduce

Review a few pages that were updated:

work pages ( /work/coachhub/, /work/adobe, /work/quarqnet )
blog posts ( 2018/01/03/introducing-herman/ )
review the examples on the Sample Placing images page: /sample/images/

Show me

github-actions · 2023-05-17T18:22:30Z

View diff of compiled files (may take a few minutes): https://github.com/oddbird/oddleventy-built/compare/main..img-helpers

stacyk · 2023-05-17T18:25:35Z

Remove or fix unused CSS classes

stacyk · 2023-05-19T13:38:31Z

@mirisuzanne this is ready for an initial review. I kept these "helpers" as classes instead of adding a data attribute incase we wanted to apply it to something other than the embed.figure or embed.img macros. Like the contact form uses extend-small.

I also took over what was the sample page called Responsive Images to show how these examples look when applied (I think it will be helpful to have a visual guide to know what is available.

This is not something in this PR, but it is related to this images sample page. I don't actually know what the difference is between the first 2 (one says no width and the other says 4410) but the secret sizes are the same and so is the visual display. The last one is using the "sizes" attribute but you can't tell from the page itself. It does have a different srcset size value. I feel like we should add some explanation to these images and why they are there, but I am not actually sure why they are all there.

mirisuzanne

Good stuff here! I just have a few questions about some of the details.

content/sample/images.md

mirisuzanne · 2023-05-19T16:28:18Z

content/sample/images.md

+access to the outer container. Also, the embed.figure macro is wider than the
+content column by default, as opposed to the `embed.img` macro.


Suggested change

access to the outer container. Also, the embed.figure macro is wider than the

content column by default, as opposed to the `embed.img` macro.

access to the outer container. Note that the `embed.figure` macro extends wider than the

content column by default, as opposed to the `embed.img` macro.

mirisuzanne · 2023-05-19T16:32:42Z

content/sample/images.md

+access to the outer container. Also, the embed.figure macro is wider than the
+content column by default, as opposed to the `embed.img` macro.
+
+### `embed.figure` without any entend class for comparison


Suggested change

### `embed.figure` without any entend class for comparison

### `embed.figure` without any extend class for comparison

Seems like this could also be called extend-small - and available to other media?

mirisuzanne · 2023-05-19T16:36:43Z

content/sample/images.md

+
+You can add the class `img-shadow` to the `embed.figure` and `embed.img` to
+display a shadow around the image. This doesn't work on figure if you have a
+caption as the shadow will be around that entire container.


Note that the shadows are not visible in dark mode?

I added an additional image for testing in both modes and upped the opacity a bit so you can see the shadow better.

mirisuzanne · 2023-05-19T16:42:47Z

src/scss/patterns/_alignment.scss

 .contain {
  display: flow-root;
 }

 [class*='align-'] {
-  margin: var(--shim) var(--inline-margin, 0);
+  margin: var(--shim) var(--inline-margin-right, var(--inline-margin, 0)) var(--shim) var(--inline-margin-left, var(--inline-margin, 0));


Suggested change

margin: var(--shim) var(--inline-margin-right, var(--inline-margin, 0)) var(--shim) var(--inline-margin-left, var(--inline-margin, 0));

margin-block: var(--shim);

margin-inline-start: var(--inline-margin-start, var(--inline-margin, 0));

margin-inline-end: var(--inline-margin-end, var(--inline-margin, 0));

A bit easier to follow? And not mixing logical/physical names?

so much easier to read when we split it out.

mirisuzanne · 2023-05-19T16:44:54Z

src/scss/patterns/_alignment.scss

+    --inline-margin-right: var(--gutter);
+
+    float: left;
+    max-width: 45%;


Are we trying to approximate 50% minus some gutter? Should we do that explicitly?

Suggested change

max-width: 45%;

max-width: calc(50% - var(--inline-margin-right) / 2);

the width is old, but I am assuming that is what it was attempting on the previous site. Either that or a little less than half of the content column looked good 😄

mirisuzanne · 2023-05-19T16:47:01Z

src/scss/patterns/_gallery.scss

@@ -47,6 +47,10 @@ figcaption,
  font-size: var(--small);
  font-style: italic;
  margin-top: var(--shim);
+
+  .extend-full & {
+    padding-inline: var(--gutter-plus);


Should we use var(--page-margin) here?

mirisuzanne · 2023-05-19T16:50:15Z

src/scss/patterns/_media.scss

+    max-width: 25vw;
+    padding-inline: calc(var(--gutter) + 2vw);


These numbers feel a bit magical and unclear to me. Why 25vw? Right now, this doesn't align with anything else on the page - should it? Is the extra gutter space meant to avoid our hanging link icons on headings? If you use this next to a paragraph rather than a heading, does the extra space still make sense?

max-width: 25vw; is a magic number. We really don't need it at all so I can remove that. As far as the padding goes, I can swap this out with page-margin. I think that will work about the same and maybe be more meaningful.

src/scss/patterns/_sizes.scss

stacyk

@mirisuzanne I think I covered all/most of your comments minus the one I had a question about (extend-small)

stacyk · 2023-05-19T17:01:58Z

src/scss/patterns/_alignment.scss

+    --inline-margin-right: var(--gutter);
+
+    float: left;
+    max-width: 45%;


the width is old, but I am assuming that is what it was attempting on the previous site. Either that or a little less than half of the content column looked good 😄

src/scss/patterns/_sizes.scss

stacyk · 2023-05-19T19:33:30Z

src/scss/patterns/_media.scss

+    max-width: 25vw;
+    padding-inline: calc(var(--gutter) + 2vw);


max-width: 25vw; is a magic number. We really don't need it at all so I can remove that. As far as the padding goes, I can swap this out with page-margin. I think that will work about the same and maybe be more meaningful.

stacyk · 2023-05-19T19:41:58Z

src/scss/patterns/_alignment.scss

 .contain {
  display: flow-root;
 }

 [class*='align-'] {
-  margin: var(--shim) var(--inline-margin, 0);
+  margin: var(--shim) var(--inline-margin-right, var(--inline-margin, 0)) var(--shim) var(--inline-margin-left, var(--inline-margin, 0));


so much easier to read when we split it out.

stacyk · 2023-05-19T19:45:11Z

content/sample/images.md

+
+You can add the class `img-shadow` to the `embed.figure` and `embed.img` to
+display a shadow around the image. This doesn't work on figure if you have a
+caption as the shadow will be around that entire container.


I added an additional image for testing in both modes and upped the opacity a bit so you can see the shadow better.

mirisuzanne · 2023-05-19T20:41:17Z

This is not something in this PR, but it is related to this images sample page. I don't actually know what the difference is between the first 2 (one says no width and the other says 4410) but the secret sizes are the same and so is the visual display. The last one is using the "sizes" attribute but you can't tell from the page itself. It does have a different srcset size value. I feel like we should add some explanation to these images and why they are there, but I am not actually sure why they are all there.

Yeah, these were for testing the responsive image stuff as I was writing it - but they're probably outdated, and were never well documented. We should add a story to actually document our responsive image handling, but these don't seem useful, so I think they can just be removed.

mirisuzanne

looks great. I would still probably vote for documenting how to extend images, and how to extend-small, while also noting that it might not work in markdown. If the has() trick even works for some browsers - it's not bad for that to be a progressive enhancement.

stacyk · 2023-06-13T22:38:52Z

@mirisuzanne I updated the sample images page to include the updated guidelines for using the img/figure macros and added that little p:has(> picture:only-child) { display: contents; } snippet along with a few styles for fallbacks. You can see it not working in Firefox and working in the others.

Firefox:

Chrome:

I removed the srcset examples and added a note to this existing image handling card: https://trello.com/c/p8iLneXl/117-document-responsive-image-handling-and-any-other-image-guidelines-that-arent-yet-on-the-sample-pages

content/sample/images.md

mirisuzanne

looks great!

src/scss/patterns/_media.scss

stacyk added 8 commits May 17, 2023 12:59

remove img-border, fix, update, replace with img-shadow

032e92f

remove unused sizes

39f8337

add media-pull

9484a27

add padding for caption on imags that are full width

1ebdb08

make sure aligned items only float at larger sizes

0c1ae36

update img to figure to allow it to extend

d598dab

add examples to image sample page

15c8557

add largest breakpoint for media-push

7e40a04

stacyk added 2 commits May 17, 2023 13:40

remove unused class and format

de0fa23

Merge branch 'main' into img-helpers

badc29e

stacyk requested a review from mirisuzanne May 19, 2023 13:27

mirisuzanne reviewed May 19, 2023

View reviewed changes

review cleanup

40c5e01

stacyk commented May 19, 2023

View reviewed changes

clarify description of media-pull

a74ee27

stacyk requested a review from mirisuzanne May 19, 2023 20:31

alignment fix

bb304f3

mirisuzanne approved these changes May 19, 2023

View reviewed changes

stacyk added 2 commits June 13, 2023 13:59

Merge branch 'main' into img-helpers

5cd799e

image utilities documentatio; add helper to nested images

44dfba3

stacyk requested a review from mirisuzanne June 13, 2023 22:33

stacyk commented Jun 13, 2023

View reviewed changes

content/sample/images.md Show resolved Hide resolved

content/sample/images.md Show resolved Hide resolved

content/sample/images.md Show resolved Hide resolved

mirisuzanne approved these changes Jun 14, 2023

View reviewed changes

src/scss/patterns/_media.scss Show resolved Hide resolved

Merge branch 'main' into img-helpers

f0fd8e0

stacyk merged commit f1e7a4c into main Jun 20, 2023
10 checks passed

stacyk deleted the img-helpers branch June 20, 2023 20:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[img-helpers] Add/Edit Image display utilities #357

[img-helpers] Add/Edit Image display utilities #357

stacyk commented May 17, 2023 •

edited

Loading

github-actions bot commented May 17, 2023

stacyk commented May 17, 2023

stacyk commented May 19, 2023

mirisuzanne left a comment

mirisuzanne May 19, 2023

mirisuzanne May 19, 2023

mirisuzanne May 19, 2023

stacyk May 19, 2023

mirisuzanne May 19, 2023

stacyk May 19, 2023

mirisuzanne May 19, 2023

stacyk May 19, 2023

mirisuzanne May 19, 2023

mirisuzanne May 19, 2023

stacyk May 19, 2023

stacyk left a comment

stacyk May 19, 2023

stacyk May 19, 2023

stacyk May 19, 2023

stacyk May 19, 2023

mirisuzanne commented May 19, 2023

mirisuzanne left a comment

stacyk commented Jun 13, 2023

mirisuzanne left a comment

		access to the outer container. Also, the embed.figure macro is wider than the
		content column by default, as opposed to the `embed.img` macro.

	### `embed.figure` without any entend class for comparison
	### `embed.figure` without any extend class for comparison

-  margin: var(--shim) var(--inline-margin-right, var(--inline-margin, 0)) var(--shim) var(--inline-margin-left, var(--inline-margin, 0));
+  margin-block: var(--shim);
+  margin-inline-start: var(--inline-margin-start, var(--inline-margin, 0));
+  margin-inline-end: var(--inline-margin-end, var(--inline-margin, 0));

	max-width: 45%;
	max-width: calc(50% - var(--inline-margin-right) / 2);

[img-helpers] Add/Edit Image display utilities #357

[img-helpers] Add/Edit Image display utilities #357

Conversation

stacyk commented May 17, 2023 • edited Loading

Description

Steps to test/reproduce

Show me

github-actions bot commented May 17, 2023

stacyk commented May 17, 2023

stacyk commented May 19, 2023

mirisuzanne left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stacyk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mirisuzanne commented May 19, 2023

mirisuzanne left a comment

Choose a reason for hiding this comment

stacyk commented Jun 13, 2023

mirisuzanne left a comment

Choose a reason for hiding this comment

stacyk commented May 17, 2023 •

edited

Loading