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

Utility or class to add .5rem margin-top on images #4791

Closed
lyubomir-popov opened this issue Jun 14, 2023 · 8 comments · Fixed by #5012
Closed

Utility or class to add .5rem margin-top on images #4791

lyubomir-popov opened this issue Jun 14, 2023 · 8 comments · Fixed by #5012
Assignees
Labels
Guest dev 🤝 Good issue to work on with guest devs Review: Code needed WG: Validated Validate working group proposals

Comments

@lyubomir-popov
Copy link
Contributor

We often need a space above the image (only the image) in layouts like these:

image

We have this locally in c.com, would be nice to have it documented and upstreamed to vanilla:

.p-image-wrapper { padding-top: $spv--small; }

@bartaz
Copy link
Contributor

bartaz commented Jun 16, 2023

This one bugs me a lot and I don't know how to approach it.

Creating a component for that purpose sounds weird. A utility/modifier on images may be a workaround, we have couple p-image--something class names currently (although by the look of them, they will be dropped soon).

The issue I see is that this "top padding" is not really part of the image, it should be part of the rule component because it comes from it and it's a bit counterintuitive that instead of rule having bottom margin, we force some elements (only images?) after it to add top margin (that we don't use anywhere else).

I experimented before with adding top margin to all images but this obviously causes some issues (especially when small images are used in lists, etc).

@lyubomir-popov
Copy link
Contributor Author

lyubomir-popov commented Jun 16, 2023

The issue is this: All text elements and components (headings, tables, accordions, tabs, etc) comes with give or take .5rem of spacing already applied (the vertical rhythm nudges). Non text blocks of content (images, videos, etc) do not have that, so they sit a little higher up. So we need something to add that amount of space to those blocks of content, but not further displace text. To me that seems a clearcut case for having a class of some sort to do it. It can't be an element qualifier as that's too broad.

@bartaz
Copy link
Contributor

bartaz commented Jun 16, 2023

@lyubomir-popov maybe a stupid question, but why do text elements have it built-in? Maybe we could remove it from text elements (move them up 0.5rem, so they will still be on the baseline) and then the rule component can simply have bottom margin?

@lyubomir-popov
Copy link
Contributor Author

lyubomir-popov commented Jun 16, 2023

Its the nudges @bartaz

They are close to, but not precisely .5rem. So for those that are .4rem (p for example) you'd need to do -.1rem padding, and paddings can't be negative. You could maybe do margin-top: -.5rem, but that won't work reliably due to margin-collapse. And if anything above the text has no margin-bottom of its own, it might get too close to the text. So there's no reliable, safe way of negating that on text elements. Much simpler to do it on the very few images or videos we add to the page. It is something we've been doing on canonical.com, it is easy to spot in reviews and fix, and takes next to no effort.

@bartaz
Copy link
Contributor

bartaz commented Jun 19, 2023

I understand that it's "next to no effort" to add a class name or a wrapper element to an image. The issue is that there is no nice way to implement it within a design system.

It's not a good component, because it's only purpose is to nudge an image couple pixels down.
It's not a good util, because it only makes sense for images.

So maybe it's a "variant" class name on an image. But will it only be images?

You mentioned @lyubomir-popov other block components that may need that. Are there any examples? I guess I've seen cards under horizontal rule, anything else may go there? Would other components have such "nudge" built in, or would they need an option for it as well?

@lyubomir-popov
Copy link
Contributor Author

applies to videos as well. not necessarily images only

@danielmutis
Copy link

@lyubomir-popov to provide design guidelines for these spacing rules. Further discussion with @bartaz

@danielmutis danielmutis added WG: Validated Validate working group proposals and removed WG: Proposal Working group proposals labels Nov 15, 2023
@bartaz bartaz self-assigned this Jan 25, 2024
@bartaz bartaz added Review: Code needed Guest dev 🤝 Good issue to work on with guest devs labels Jan 25, 2024
@bartaz
Copy link
Contributor

bartaz commented Feb 28, 2024

Scope:

We could implement it as a new component called "media container", possibly as simple as:

.p-media-container {
   margin-top: $sp-unit;
}

It should be documented on Images page https://vanillaframework.io/docs/patterns/images.
An example should show an image wrapped in this new container placed under a rule component:

<hr class="p-rule" />
<div class="p-media-container">
   <img  />
</div>

possibly should be mentioned on Rule component page, as it's mostly needed for situations when we place an image under a rule component (https://vanillaframework.io/docs/patterns/rule)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Guest dev 🤝 Good issue to work on with guest devs Review: Code needed WG: Validated Validate working group proposals
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants