Remove padding-bottom in cta-block

[mcslayer]

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:

• [x] 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]

mcslayer is not a collaborator of the repo

[bartaz]

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]

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:

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]

@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]

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.

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]

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: https://github.com/canonical/vanilla-framework/blob/ce8551bdec28187663d3de35f199db5829509cb6/scss/_base_typography-definitions.scss#L153-L165

[bartaz]

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]

.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]

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]

@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:

After:

Edit: This doesn't solve the problem that we ideally shouldn't place anchors inside <div> and shouldn't place buttons inside <p>.

[jmuzina]

Closing to further discussion in the original issue, #5397 - we need a bit more alignment before this is ready to be picked up.