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

Improved table element styles & fixed table styles inside asides #2064

Merged
merged 11 commits into from
Jul 5, 2024

Conversation

SnowDingo
Copy link
Contributor

@SnowDingo SnowDingo commented Jun 29, 2024

This PR brings two changes to Astro Starlight:

  1. It closes Tables in Asides #1725, which addresses the broken style in the table inside the aside element.
  2. It also adds new styling to the table element in Astro, with most of this part of the code from @delucis.

Here are sample images of the new styling:
スクリーンショット 2024-06-29 9 30 42
スクリーンショット 2024-06-29 9 30 47
スクリーンショット 2024-06-29 9 31 13
スクリーンショット 2024-06-29 9 31 23
スクリーンショット 2024-06-29 9 32 16
スクリーンショット 2024-06-29 9 34 06
スクリーンショット 2024-06-29 9 35 08
スクリーンショット 2024-06-29 9 35 14
スクリーンショット 2024-06-29 9 35 22
スクリーンショット 2024-06-29 9 35 29
スクリーンショット 2024-06-29 9 35 37
スクリーンショット 2024-06-29 9 35 48

Copy link

changeset-bot bot commented Jun 29, 2024

🦋 Changeset detected

Latest commit: c8da560

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@astrojs/starlight Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

Copy link

vercel bot commented Jun 29, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated (UTC)
starlight ✅ Ready (Inspect) Visit Preview Jul 5, 2024 9:28am

@github-actions github-actions bot added the 🌟 core Changes to Starlight’s main package label Jun 29, 2024
@github-actions github-actions bot added the 📚 docs Documentation website changes label Jun 29, 2024
@astrobot-houston
Copy link
Collaborator

astrobot-houston commented Jun 29, 2024

Lunaria Status Overview

🌕 This pull request will trigger status changes.

Learn more

By default, every PR changing files present in the Lunaria configuration's files property will be considered and trigger status changes accordingly.

You can change this by adding one of the keywords present in the ignoreKeywords property in your Lunaria configuration file in the PR's title (ignoring all files) or by including a tracker directive in the merged commit's description.

Tracked Files

Locale File Note
en guides/tables-test.md Source added, will be tracked.
Warnings reference
Icon Description
🔄️ The source for this localization has been updated since the creation of this pull request, make sure all changes in the source have been applied.

@delucis delucis added the 🌟 minor Change that triggers a minor release label Jun 29, 2024
Copy link
Member

@delucis delucis left a comment

Choose a reason for hiding this comment

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

Thank you @SnowDingo! I made some changes as I think there were some conflicts between the styles you applied and other recent CSS changes to other bits of Starlight and I removed the background striping to match my example StackBlitz.

I’ve temporarily added a test page to the Starlight docs on this branch so people can view the result for a range of examples and check it in both light and dark mode: https://starlight-git-fork-snowdingo-main-astrodotbuild.vercel.app/guides/tables-test/

I’m personally happy with this, but would love to hear feedback from anyone else who has thoughts!

@lorenzolewis
Copy link
Contributor

lorenzolewis commented Jun 29, 2024

For me it seems like the blue border is a bit too red and in the purple-ish territory instead of blue.

Light Dark
image image

Copy link
Member

@sarah11918 sarah11918 left a comment

Choose a reason for hiding this comment

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

Looks great! Some really nice styling here!

@evadecker
Copy link
Contributor

evadecker commented Jun 29, 2024

Agree with @lorenzolewis about the blue dividers having a purple tint.

The header row divider feels too strong; I would lower the opacity or match the tint of the header text for the aside.

The final divider of the table is subtly lighter than all the other dividers.

In addition, it could be nice to remove the start and end inline padding of the first and last column of the table so the text aligns with the dividers and with other surrounding paragraphs!

image

@delucis
Copy link
Member

delucis commented Jun 29, 2024

Thanks for the reviews folks — super helpful! I will play around with the header border and inline padding @evadecker 🙌

The final divider of the table is subtly lighter than all the other dividers.

@evadecker Could you share a bit more about the browsers/hardware you’re using? I’ve checked a bunch of different browsers on macOS and can’t see the same thing. Wondering if its a subpixel antialiasing effect at a specific resolution 🤔

@evadecker
Copy link
Contributor

evadecker commented Jun 29, 2024

Thanks for the reviews folks — super helpful! I will play around with the header border and inline padding @evadecker 🙌

The final divider of the table is subtly lighter than all the other dividers.

@evadecker Could you share a bit more about the browsers/hardware you’re using? I’ve checked a bunch of different browsers on macOS and can’t see the same thing. Wondering if its a subpixel antialiasing effect at a specific resolution 🤔

First noticed on iOS Safari and the issue is present on macOS Safari as well. Not present on Chrome.

The issue seems to stem from border-collapse: collapse in combination with the semitransparent border colors.

Removing that rule and adding border-spacing: 0 fixes the issue.

CleanShot.2024-06-29.at.10.00.41.mp4

@delucis
Copy link
Member

delucis commented Jun 29, 2024

OK, made two more changes based on @evadecker’s feedback:

  1. Removed the inline padding at the start and end of each row.
  2. Simplified the colours by using the same colour for the heading row divider as the rest of the rows.
Before After
starlight-git-fork-snowdingo-main-astrodotbuild vercel app_guides_tables-test_ localhost_4321_guides_tables-test_

Copy link
Contributor

@evadecker evadecker left a comment

Choose a reason for hiding this comment

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

Looks great! Excited to see this go live!

Copy link
Contributor

@lorenzolewis lorenzolewis left a comment

Choose a reason for hiding this comment

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

Yes, looks perfect now for me in the always-problematic browser that is Safari 😉

@SnowDingo
Copy link
Contributor Author

SnowDingo commented Jun 30, 2024

Yes, looks perfect now for me in the always-problematic browser that is Safari 😉

Does anyone know if this PR is ready to go?
If not, since I couldn't catch up to the comments, it would be conducive if someone could create a list of changes we must make before merging this PR.

@libreom
Copy link

libreom commented Jun 30, 2024

Screenshot_20240630-072856_Brave
Screenshot_20240630-072907_Brave
Not so good

@SnowDingo SnowDingo requested a review from HiDeoo June 30, 2024 23:14
@delucis
Copy link
Member

delucis commented Jul 1, 2024

Does anyone know if this PR is ready to go?

@SnowDingo Yes! Good to go. We will merge it when we are ready for the next release. Should be later this week. Thanks again!

@SnowDingo
Copy link
Contributor Author

Does anyone know if this PR is ready to go?

@SnowDingo Yes! Good to go. We will merge it when we are ready for the next release. Should be later this week. Thanks again!

@delucis , no problem👍
I can't wait to see our new styling in starlight!!

@delucis delucis linked an issue Jul 5, 2024 that may be closed by this pull request
1 task
@delucis delucis changed the title [Closes issue #1725 ] Improved general styling of the table element in Starlight & fixed broken styling of table element inside an aside component. Improved table element styles & fixed table styles inside asides Jul 5, 2024
@delucis delucis merged commit c5b47cb into withastro:main Jul 5, 2024
14 checks passed
@astrobot-houston astrobot-houston mentioned this pull request Jul 5, 2024
@HiDeoo HiDeoo mentioned this pull request Jul 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🌟 core Changes to Starlight’s main package 📚 docs Documentation website changes 🌟 minor Change that triggers a minor release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Tables in Asides
8 participants