-
Notifications
You must be signed in to change notification settings - Fork 169
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
base: main
Are you sure you want to change the base?
Conversation
mcslayer is not a collaborator of the repo |
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. |
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 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 |
@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 |
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? |
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: vanilla-framework/scss/_base_typography-definitions.scss Lines 153 to 165 in ce8551b
|
OK. There is a bit confusion here. CTA Block, as a component, may contain buttons and/or links. Any combination of those. How are we normally doing link next to a button? I has always been just I suppose we only have a problem when CTA block only contains text links? We could in such case wrap them in paragraph? 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. |
this definitely seems confusing. If it extends a paragraph, then we shouldn't have a @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). |
On one hand I understand what the intention is there with 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)? |
We could wrap the quote wrapper's CTA contents with a |
Done
Remove padding-bottom in cta-block, based on issue #5397
Fixes #5397
QA
CTA Block
pageCheck 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:
Feature 🎁
,Breaking Change 💣
,Bug 🐛
,Documentation 📝
,Maintenance 🔨
.package.json
should be updated relative to the most recent release, following semver convention: