Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

poc: Add bottom margin to text components #1903

Draft
wants to merge 11 commits into
base: develop
Choose a base branch
from

Conversation

VincentSmedinga
Copy link
Contributor

@VincentSmedinga VincentSmedinga commented Mar 4, 2025

Describe the pull request

Thank you for contributing to the project!
Please use this template to help us handle your PR smoothly.

What

  • This implements vertical margins between text components.
  • The amount of space has been designed between each combination of 22 components.
  • I’ve implemented 6 of them (Heading, Image, Standalone Link, Ordered List, Paragraph and Unordered List) and added the News Page with which the spacing was designed.
  • I decreased the space between Headings and following body text. I think this groups the sections better, making them easier to scan. The new line heights are active as well.
  • I’ve removed manual space classes from the Home Page and Article Page and tweaked some tokens for Card and Lists.

Links to full-screen pages

Why

To make it easy for our users to apply space in prose consistently.

How

  • I’ve created Sass placeholders for the various margin options.
  • And used them in the 6 components mentioned above. I’m using the :has selector to let the bottom margin of a component depend on the component that follows it. This is as designed, logical, and prevents the use of :last-child which is error-prone for this context.
  • I changed the value of xl space from 48 to 56 and added an xxl of 84, both according to the design. This could be the start of merging Component Space and Grid Space, although both currently shrink and grow quite differently.

Checklist

Before submitting your pull request, please ensure you have done the following. Check each checkmark if you have done so or if it wasn't necessary:

  • Add or update unit tests
  • Add or update documentation
  • Add or update stories
  • Add or update exports in index.* files
  • Start the PR title with a Conventional Commit prefix, as explained here.

Additional notes

  • Marked this as a POC because of a few out-of-scope changes.
    This generates quite a few lines of CSS, but I see no way around it. We must use our classes and explicitly address all combinations. The built stylesheet increased from 99 to 103 kB, so I guess around 1 kB per component that uses this.
  • We could consider making a medium bottom margin the default for our components, but we would need to add a way to negate that for our users, and the ‘last child’ problem may re-emerge.
  • Components referencing other components’ class names (which is not new, but at a larger scale) will hinder our ability to package our components separately in the future.
  • Sass placeholders seem helpful in making maintenance more efficient. We can even take a step further if we can group components by the amount of space they use, e.g.
.ams-fieldset__legend,
.ams-heading,
.ams-label {
  @extend %headings;
}

.ams-paragraph,
.ams-ordered-list,
.ams-unordered-list {
  @extend %body-text;
}

.ams-action-group,
.ams-button,
.ams-link--standalone {
  @extend %actions;
}

Then this would create all the selectors to add space between all kinds of headings and all kinds of body text:

%headings :has(+%body-text) {
  @extend %ams-mb--medium;
}

@VincentSmedinga VincentSmedinga requested review from dlnr and Bondt007 March 4, 2025 12:53
@VincentSmedinga VincentSmedinga requested a review from a team as a code owner March 4, 2025 12:53
@VincentSmedinga VincentSmedinga marked this pull request as draft March 4, 2025 12:53
@github-actions github-actions bot temporarily deployed to demo-DES-1180-vertical-space March 5, 2025 08:21 Destroyed
# Conflicts:
#	packages/css/src/components/card/card.scss
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

1 participant