Fixed issue #1725. Added styling to table inside the aside component.

[SnowDingo]

Description

• Closes #1725
• This PR resolves the issue of styling of a table inside an aside element.
• One example of the visual change made by this PR is the screenshot below.

Right now with this PR, the table inside the aside element looks like the screenshots below:

[changeset-bot[bot]]

⚠️ No Changeset found

Latest commit: 1063afa131c8c76586299c4814ec088edf52f780

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

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

[vercel[bot]]

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

Name	Status	Preview	Updated (UTC)
starlight	✅ Ready (Inspect)	Visit Preview	Apr 29, 2024 11:41pm

[astrobot-houston]

Hello! Thank you for opening your first PR to Starlight! ✨

Here’s what will happen next:

1. Our GitHub bots will run to check your changes. If they spot any issues you will see some error messages on this PR. Don’t hesitate to ask any questions if you’re not sure what these mean!

2. In a few minutes, you’ll be able to see a preview of your changes on Vercel 🤩

3. One or more of our maintainers will take a look and may ask you to make changes. We try to be responsive, but don’t worry if this takes a few days.

[astrobot-houston]

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	index.mdx	Source changed, localizations will be marked as outdated.

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.

[HiDeoo]

Thanks for your contribution :raised_hands:

I did not get a look at the code yet but from a quick look on the results, this is looking like a great improvement. On top of background color changes, I'm also wondering if some work on border colors could be also beneficial, thinking for example on the case of caution aside in light mode. Do you have any thoughts on that?

I'm planning to take a closer look with more eyes this Thursday during Astro Talking & Doc'ing session on Discord as it's a great opportunity to discuss it with the community and have more eyes on it which is great for visual changes. If you're available, feel free to join us there, it would be great to have you there to discuss it with us.

[SnowDingo]

Thanks for your contribution 🙌

I did not get a look at the code yet but from a quick look at the results, this is looking like a great improvement. On top of background color changes, I'm also wondering if some work on border colors could be also beneficial, thinking for example on the case of caution aside in light mode. Do you have any thoughts on that?

I'm planning to take a closer look with more eyes this Thursday during Astro Talking & Doc'ing session on Discord as it's a great opportunity to discuss it with the community and have more eyes on it which is great for visual changes. If you're available, feel free to join us there, it would be great to have you there to discuss it with us.

@astrobot-houston
Thanks for your review of this PR. I couldn't have a chance to join the Astro talking on Thursday, but I believe adding a border to styling is a great improvement. I will work on adding the border as soon as possible.

[HiDeoo]

I couldn't have a chance to join the Astro talking on Thursday

No worries at all. To summarize what happened, we mostly did a visual review, on various OS, browsers, using both the light and dark themes, tried various scenarios, etc. and the conclusion was that the original issue is clearly addressed by your changes and amazing work.

Regarding the borders, this is something that also came up in the discussion, but after trying various options, we think that all table borders may need some work, not only the one in asides. This is a more general issue that we could address in a separate PR after some discussion identifying the best approach, what are the general issues with the current borders, and how we can improve them.

Considering all this, I think the best approach would be to keep the current PR as-is, have a more code oriented review and ship the current changes as they fix the original issue. Then, we can start discussing the general border issue in a separate PR.

What do you think?

[SnowDingo]

No worries at all. To summarize what happened, we mostly did a visual review, on various OS, and browsers, using both the light and dark themes, tried various scenarios, etc. and the conclusion was that the original issue was addressed by your changes and amazing work.

Regarding the borders, this is something that also came up in the discussion, but after trying various options, we think that all table borders may need some work, not only the one in the asides. This is a more general issue that we could address in a separate PR after some discussion identifying the best approach, what are the general issues with the current borders, and how we can improve them.

Considering all this, I think the best approach would be to keep the current PR as-is, have a more code-oriented review, and ship the current changes as they fix the original issue. Then, we can start discussing the general border issue in a separate PR.

What do you think?

I agree with you that we should fix the general styling of the table. I was wondering whether this PR should be shipped the way it is or if it needs some changes on certain parts of the code. If there need to be some minor changes, would it be possible for you to list them so I can try fixing them?

Thank you in advance,

Snowdingo.

[delucis]

Hey all! Thanks so much for digging into this. I took some time to play around with it a bit and think about it and wondered if maybe one approach could be switching to a table design that doesn’t rely on these background colours at all.

For example maybe something like the experiments here: https://stackblitz.com/edit/github-2qdnrc?file=src%2Ftables.css

Might need some more discussions with team as these styles are quite different, but could be a nice way to both simplify things and solve this issue.

[SnowDingo]

@delucis

Sorry for the late reply. I have been thinking about a way to solve this issue most simply, and I agree with you that the table style should be simple. I was wondering if this PR should be accepted with its current situation so at least the issue is fixed. Then, I would suggest we make a separate PR that will fix the general table styling.

Many thanks for considering my request.

Best Regards, Snowdingo

[delucis]

Sorry for the late reply.

No worries! I appreciate you helping out 💖

I was wondering if this PR should be accepted with its current situation so at least the issue is fixed. Then, I would suggest we make a separate PR that will fix the general table styling.

I think I would prefer to fix this in a single PR rather than making two consecutive PRs and have the table styling change for users twice, especially as currently this PR introduces new background shades and we might risk someone starting to use those, only for us to then remove them again.

Are you interested in updating this PR with the styles linked above? Or would you like me to have a go at doing that?

[SnowDingo]

Sorry for the late reply.

No worries! I appreciate you helping out 💖

I was wondering if this PR should be accepted with its current situation so at least the issue is fixed. Then, I would suggest we make a separate PR that will fix the general table styling.

I think I would prefer to fix this in a single PR rather than making two consecutive PRs and have the table styling change for users twice, especially as currently this PR introduces new background shades and we might risk someone starting to use those, only for us to then remove them again.

Are you interested in updating this PR with the styles linked above? Or would you like me to have a go at doing that?

I now believe it will be better to close this PR. Then, I will quickly fix the general styling of the table in Starlight and will create a new PR that fixes not only issue #1725 but also the table styling issue that we just discussed. Do you think this is a good idea?

[delucis]

I now believe it will be better to close this PR. Then, I will quickly fix the general styling of the table in Starlight and will create a new PR that fixes not only issue https://github.com/withastro/starlight/issues/1725 but also the table styling issue that we just discussed.

However you prefer! If a fresh PR feels better, you’re welcome to make one, but if you prefer to reuse this one, that’s fine too.

[SnowDingo]

However you prefer! If a fresh PR feels better, you’re welcome to make one, but if you prefer to reuse this one, that’s fine too.

Dear @delucis,

I'll make sure to create a new pull request addressing the issues with the sample styling codes you provided on StackBlitz. Thanks for your suggestions on improving this PR.

Best regards, SnowDingo