Skip to content

Update table styles so nhsuk-table has bottom margin #1005

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

Merged
merged 4 commits into from
Apr 8, 2025

Conversation

edwardhorsford
Copy link
Contributor

@edwardhorsford edwardhorsford commented Sep 6, 2024

This fixes some inconsistency with the table styles.

The current table styles rely on a bottom margin being applied to the table element rather than nhsuk-table. The responsive table variant resets this so it has no bottom margin.

The styles include a nhsuk-table-container class that's presumably meant to wrap tables, but this is not used or documented on the design system site. I might guess it was added for a specific use case. For now I've left it, but teams shouldn't need to use it to get the correct bottom margin.

This applies some basic styles to nhsuk-table similar to how govuk-table styles it. If nhsuk-table-container can still be used.

I think this should probably have no visual impact for most users. But if any had added extra margin to account for the responsive table not having margin, then that could double. It now sets the font too - which it's possible could conflict with a non standard font.

@frankieroberto
Copy link
Contributor

frankieroberto commented Sep 10, 2024

Might be worth opening a second PR to remove .nhsuk-table-container - looks like it’s no longer used for responsive tables at all (those use .nhsuk-table--responsive)?

Do you know about this at all @anandamaryon1?

@anandamaryon1
Copy link
Collaborator

Looks good to me, I've not seen the .nhsuk-table-container being used anywhere but looking at the styles it could be used to create a scrolling table?

Perhaps leave it in for now.

Based on this, the responsive table example will need both classes, so the block class adding too (.nhsuk-table), for the margin bottom.

Note to self: will need to check service manual aligns.

colinrotherham
colinrotherham previously approved these changes Mar 12, 2025
Copy link
Contributor

@colinrotherham colinrotherham left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! 🙌

I've added regression test images and made a tweak below

Unfortunately requiring both nhsuk-table and nhsuk-table-responsive classes would be a breaking change for non-Nunjucks users, but this works:

- .nhsuk-table {
+ .nhsuk-table,
+ .nhsuk-table-responsive {

@colinrotherham
Copy link
Contributor

Updated the reference images again (for the new sizes)

@colinrotherham colinrotherham self-requested a review April 3, 2025 17:32
frankieroberto
frankieroberto previously approved these changes Apr 4, 2025
colinrotherham
colinrotherham previously approved these changes Apr 4, 2025
@colinrotherham colinrotherham dismissed stale reviews from frankieroberto and themself via 48b3915 April 7, 2025 13:00
@colinrotherham
Copy link
Contributor

Updated reference images again since #1170 merged

@Smartin684 Smartin684 requested a review from anandamaryon1 April 8, 2025 10:08
@colinrotherham
Copy link
Contributor

Just reviewed this with @anandamaryon1 and we're going to get it merged

Thanks @edwardhorsford

@colinrotherham colinrotherham self-requested a review April 8, 2025 15:40
@colinrotherham colinrotherham merged commit 953118b into main Apr 8, 2025
10 checks passed
@github-project-automation github-project-automation bot moved this from Needs review to Done in Service Manual Sprint Board Apr 8, 2025
@colinrotherham colinrotherham deleted the update-table-styles branch April 8, 2025 15:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Development

Successfully merging this pull request may close these issues.

4 participants