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

Remove padding-bottom in cta-block #5398

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

mcslayer
Copy link
Contributor

@mcslayer mcslayer commented Oct 23, 2024

Done

Remove padding-bottom in cta-block, based on issue #5397

Fixes #5397

QA

  • Open CTA Block page

Check if PR is ready for release

If this PR contains Vanilla SCSS code changes, it should contain the following changes to make sure it's ready for the release:

  • PR should have one of the following labels to automatically categorise it in release notes:
    • Feature 🎁, Breaking Change 💣, Bug 🐛, Documentation 📝, Maintenance 🔨.
  • Vanilla version in package.json should be updated relative to the most recent release, following semver convention:
    • if CSS class names are not changed it can be bugfix release (x.x.X)
    • if CSS class names are changed/added/removed it should be minor version (x.X.0)
    • see the wiki for more details
  • Any changes to component class names (new patterns, variants, removed or added features) should be listed on the what's new page.

@webteam-app
Copy link

mcslayer is not a collaborator of the repo

@bartaz
Copy link
Member

bartaz commented Oct 24, 2024

Thanks @mcslayer for another contribution. We want to first consult with designer if this is an intended change. There is a bit of inconsistency between designs in Figma and our component in this regard.

@jmuzina
Copy link
Member

jmuzina commented Oct 24, 2024

I think I found where that bottom padding came from: quote wrapper has a link-only CTA that depends on padding-bottom to space it away from content beneath it

Before/after:
Screenshot 2024-10-24 at 10 50 08 AM

For our higher order components like quote block, we do not write any Sass; we just consume other styles. So, a good way of fixing this is probably to wrap the cta block in the quote wrapper in a .p-section--shallow

@jmuzina
Copy link
Member

jmuzina commented Oct 24, 2024

@lyubomir-popov @dgtlntv @mattea-turic Can you have a look at the CTA block component on its docs, usage in hero, usage in quote wrapper, and usage in tiered list and let us know what the correct bottom spacing is for those cases?

My suspicion is that the padding-bottom was only really needed for the quote wrapper (see comment), and it can be fixed by wrapping in a shallow section there instead of relying on padding-bottom from the CTA itself. All the other use cases can benefit from decreased whitespace by removing the padding-bottom.

@lyubomir-popov
Copy link
Contributor

lyubomir-popov commented Oct 25, 2024

we have our example wrong: we should not place anchor elements (as they are inline) directly in divs. We wrap them in paragraphs or headings, which applies the needed padding-top/margin-bottom to align the text to the baseline grid and add whatever space is needed after.

Screenshot 2024-10-25 at 9 43 06 AM

Alternatively, maybe we ca nhavecan have a class that applies the paragraph stylignstyling to simple anchors, to avoid the nesting - what do you think @bartaz?

@jmuzina
Copy link
Member

jmuzina commented Oct 25, 2024

maybe we ca nhavecan have a class that applies the paragraph stylignstyling to simple anchors, to avoid the nesting

With this already released, we should continue supporting .p-cta-block to avoid a breaking change - maybe the easiest way to do this is do remove the bottom spacing on the .p-cta-block class itself, and make .p-cta-block extend paragraph styling like so:

.p-cta-block {
  @extend %paragraph;
  // ......
}

Which would extend this behavior:

%paragraph {
@extend %common-default-text-properties;
margin-bottom: map-get($sp-after, p) - map-get($nudges, p);
&:not([class*='p-heading--']):not([class*='p-muted-heading']) + & {
margin-top: -#{$sp-unit};
}
&.u-no-margin--bottom {
@extend %u-no-margin--bottom--default-text;
}
}

@bartaz
Copy link
Member

bartaz commented Oct 25, 2024

OK. There is a bit confusion here.

CTA Block, as a component, may contain buttons and/or links. Any combination of those.
If it contains buttons, it should not add additional spacing, right? We don't put buttons in paragraphs.
But we want additional spacing for links alone?

How are we normally doing link next to a button? I has always been just <div><button>Button</button> <a>Link</a></div>.
Adding paragraph styling for a link also seems wrong. Links are inline elements they should not have spacing of their own.

I suppose we only have a problem when CTA block only contains text links? We could in such case wrap them in paragraph?
But the issue is we don't know in the pattern what they are. Because they are part of content. So I don't think we can detect what kind of content is provided in the slot to adjust.

We would either need to depend on devs to put the paragraph (or whatever is needed for correct spacing), or have a separate propery for it or something.

@lyubomir-popov
Copy link
Contributor

.p-cta-block {
  @extend %paragraph;
  // ......
}

this definitely seems confusing. If it extends a paragraph, then we shouldn't have a button inside.

@bartaz I guess what I am proposing is .p-text-link class that makes inline anchors into text that aligns with the baseline grid (which is something we expect of all text we display, regardless of how a developer coded it).

@bartaz
Copy link
Member

bartaz commented Oct 30, 2024

On one hand I understand what the intention is there with p-text-link, but how would devs know when to use it?

We would need to require devs to add this new class name to links if they are not in paragraph, but not add it, when link in inside of the other text elements. It is gonna be very easily forgotten.

Not sure what is the good way to get around that.

@jmuzina Would it help at all, if for the case of Quote component we don't use CTA block component at all (if it's never meant to contain a button anyway, just a link)?

@jmuzina
Copy link
Member

jmuzina commented Oct 30, 2024

@jmuzina Would it help at all, if for the case of Quote component we don't use CTA block component at all (if it's never meant to contain a button anyway, just a link)?

@bartaz

We could wrap the quote wrapper's CTA contents with a <p> and preface the <p> with hr.p-rule--muted.is-fixed-width for roughly the same look. There is a slight change in top margin since we're now using paragraph, but this actually puts the CTA text on the baseline (it currently is not).

Before:
Screenshot 2024-10-30 at 3 23 31 PM

After:
Screenshot 2024-10-30 at 3 23 29 PM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

CTA Block: Excessive bottom padding
5 participants