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

Added Dark Theme #92

Merged
merged 5 commits into from
Sep 15, 2024
Merged

Conversation

jitendravjh
Copy link
Contributor

@jitendravjh jitendravjh commented Aug 11, 2024

Closes #91

Copy link
Contributor

Preview the changes: https://turinglang.org/pr-previews/92
Please avoid using the search feature and navigation bar in PR previews!

@shravanngoswamii shravanngoswamii self-requested a review August 11, 2024 19:08
@shravanngoswamii
Copy link
Member

shravanngoswamii commented Aug 11, 2024

I think preview workflow is not working, let me see it first!

@shravanngoswamii
Copy link
Member

Text colors in lists is still white, is it intended to be like that?
image

@jitendravjh
Copy link
Contributor Author

Text colors in lists is still white, is it intended to be like that?

fixed it!

@shravanngoswamii
Copy link
Member

I think text color in navbar will look much better if it's white!

@shravanngoswamii
Copy link
Member

Everything looks good to me, maybe @yebai would like to see colours once —
Main Site: https://turinglang.org/pr-previews/92
Docs Site: https://turinglang.org/docs/pr-previews/509

@yebai
Copy link
Member

yebai commented Aug 25, 2024

Thanks @jitendravjh and @shravanngoswamii! Maybe @penelopeysm can have a final say here?

@penelopeysm penelopeysm self-requested a review August 25, 2024 17:01
Copy link
Member

@penelopeysm penelopeysm left a comment

Choose a reason for hiding this comment

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

Hi @jitendravjh, this is great work :) I like the colours a lot (despite being an always-light-mode girl)

There are some small inconsistencies in the CSS variables between the cosmo and solar themes. You can see these here:

For example, solar sets

$breadcrumb-padding-y:              .375rem !default;
$breadcrumb-padding-x:              .75rem !default;

This leads to some moving around when the colour scheme is changed (warning: flashing colours)

css.mov

There are a couple of ways to get around this. One is to specifically override SCSS variables like the breadcrumb-padding ones.

The other is to use cosmo as a base theme and override every single colour variable. Basically, copy all the colour variables over from the base solar SCSS file into theming/theme-dark.scss (and replace any of them that you want to tweak, such as $body-color).

I would lean towards the latter, because using cosmo as a base will guarantee that we have all the non-colour CSS variables exactly the same. Furthermore, this will continue to hold true even if the Bootstrap themes are updated, meaning that it reduces the future maintenance burden.

@shravanngoswamii
Copy link
Member

@jitendravjh Any updates?

@shravanngoswamii
Copy link
Member

The other is to use cosmo as a base theme and override every single colour variable. Basically, copy all the colour variables over from the base solar SCSS file into theming/theme-dark.scss (and replace any of them that you want to tweak, such as $body-color).

This looks much better approach to me, using this there will no to minimal chances in theme and color inconsistencies in future!

@jitendravjh
Copy link
Contributor Author

Apologies for the delay. The requested changes have been implemented. Please review and let me know if any further adjustments are needed.
@penelopeysm @shravanngoswamii

Main Site: https://turinglang.org/pr-previews/92
Docs Site: https://turinglang.org/docs/pr-previews/509

Copy link
Member

@penelopeysm penelopeysm 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!

@shravanngoswamii
Copy link
Member

Thank you, @jitendravjh!

@shravanngoswamii shravanngoswamii merged commit 911471d into TuringLang:main Sep 15, 2024
3 checks passed
github-actions bot added a commit that referenced this pull request Sep 15, 2024
@yebai
Copy link
Member

yebai commented Sep 15, 2024

Thanks, @jitendravjh and @shravanngoswamii, for the nice improvements!

@jitendravjh jitendravjh deleted the jitendravjh/theme branch September 17, 2024 03:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Dark Mode for website
4 participants