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
Fixed issue #1725. Added styling to table inside the aside component. #1796
base: main
Are you sure you want to change the base?
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
Hello! Thank you for opening your first PR to Starlight! ✨ Here’s what will happen next:
|
Lunaria Status Overview🌕 This pull request will trigger status changes. Learn moreBy default, every PR changing files present in the Lunaria configuration's You can change this by adding one of the keywords present in the Tracked Files
Warnings reference
|
Thanks for your contribution 🙌 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. |
@astrobot-houston |
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? |
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. |
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: 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. |
Description
Right now with this PR, the table inside the aside element looks like the screenshots below: