feat: sphinx dark theme #1146

danielhe4rt · 2024-07-22T01:29:25Z

Hey folks!

After some hours hacking into the Sphinx, I managed to add dark theme by adding TailwindCSS.

Motivation

Since I'm working mostly at night in these past days, my sight get tired faster than during the day by the fact of the screen brightness + white theme. So I've decided to implement this to make it an option not only for me, but for the other users that probably are looking for this feature.

This PoC/Concept was designed by @nexturhe4rt (cheers).

Here I simply applied the colors and enhance a couple of css mostly in the navigation.

I'd like to hear some feedbacks on that!

PoC/Concept

Actual Implementation

Transition gif

Hero/Landing

Content page

Mobile Toggle:

tzach · 2024-07-22T02:00:48Z

#251

tzach · 2024-07-22T02:03:00Z

Thanks, @danielhe4rt. It looks suitable for my untrained eye, but I would like a professional designer to review it.

danielhe4rt · 2024-07-22T02:37:23Z

@tzach just to be clear: Nextur is a Senior UX/UI. I think we need a professional front-end to review it. Also I'll test with the main ScyllaDB documentation and see if I missed something.

He got some cool ideas but I didn't risked to implement since I'm not a front-end :p

I'll ask some FE folks to check the merge request too.

Luisnadachi · 2024-07-22T02:40:31Z

src/css/main.scss

+body {
+  @apply dark:bg-surface-base dark:bg-gradient-to-tl dark:from-gradient-start dark:to-gradient-end;
+}
+


Before and after body, is alright have two lines?

Luisnadachi · 2024-07-22T02:41:18Z

src/css/components/_labels.scss

  }

  &--caution {
    background-color: $color-background-label-caution;
+    @apply dark:border-[#FFAB00EB] dark:bg-[#FFAB006B];
+
+


Here is alright two lines?

Luisnadachi · 2024-07-22T02:47:08Z

src/css/components/_footer.scss


    img {
+      @apply dark:scylla-icon-svg;


idk if is the point of change about the footer, but, the gif show scylla logo dark, wouldn't it be better put the scylla logo white? Print for example

I missed this one. I'll fix it later, thanks!

Daniel-Boll · 2024-07-22T15:05:25Z

src/css/base/_index.scss

+
+
+


Unnecessary lines

Daniel-Boll · 2024-07-22T15:08:11Z

src/css/base/_reset.scss

+code {
+  @apply py-0.5 px-2 border-8 dark:text-typography-titlelink dark:bg-navigation-base bg-[#f7f8f9] !important;
+}
+
+.highlight pre {
+    @apply dark:bg-navigation-base dark:text-typography-title !important;
+}


Apparently !important tags should be escaped as per preprocessors docs

Suggested change

code {

@apply py-0.5 px-2 border-8 dark:text-typography-titlelink dark:bg-navigation-base bg-[#f7f8f9] !important;

}

.highlight pre {

@apply dark:bg-navigation-base dark:text-typography-title !important;

}

code {

@apply py-0.5 px-2 border-8 dark:text-typography-titlelink dark:bg-navigation-base bg-[#f7f8f9] #{!important};

}

.highlight pre {

@apply dark:bg-navigation-base dark:text-typography-title #{!important};

}

There are plenty other places where this is used, if it is to be fixed those should be found, I won't pin point them as it may be unnecessary.

Daniel-Boll · 2024-07-22T15:13:27Z

src/css/components/_header.scss

+.dark-theme-toggle {
+  @apply dark:text-white text-black p-2 dark:bg-navigation-base bg-gray-200 rounded focus:outline-none;
+  display: flex;
+  flex-direction: row;
+  margin-right: 12px
+}
+


Why not commit all to tailwind?

Suggested change

.dark-theme-toggle {

@apply dark:text-white text-black p-2 dark:bg-navigation-base bg-gray-200 rounded focus:outline-none;

display: flex;

flex-direction: row;

margin-right: 12px

}

.dark-theme-toggle {

@apply flex flex-row mr-3 dark:text-white text-black p-2 dark:bg-navigation-base bg-gray-200 rounded focus:outline-none;

}

cherryramatisdev · 2024-07-22T21:46:02Z

sphinx_scylladb_theme/side-nav.html

@@ -1,4 +1,4 @@
-<div id="side-nav" class="side-nav custom-scroll-bar" data-closable data-toggler=".show">
+<div id="side-nav" class="side-nav @apply dark:bg-navigation-base custom-scroll-bar" data-closable data-toggler=".show">


Suggested change

<div id="side-nav" class="side-nav @apply dark:bg-navigation-base custom-scroll-bar" data-closable data-toggler=".show">

<div id="side-nav" class="side-nav dark:bg-navigation-base custom-scroll-bar" data-closable data-toggler=".show">

I don't think you need @apply here since you're already on a "class"

cherryramatisdev · 2024-07-22T21:53:00Z

src/js/dark-theme.js

+        el.addEventListener('click', () => {
+            if (document.documentElement.classList.contains('dark')) {
+                document.documentElement.classList.remove('dark');
+                localStorage.setItem('dark-mode', 'false');


Just a small tip, but it should be easier to handle values using json stringify and json parse

Suggested change

localStorage.setItem('dark-mode', 'false');

localStorage.setItem('dark-mode', JSON.stringify(false));

cherryramatisdev · 2024-07-22T21:53:15Z

src/js/dark-theme.js

+                toggle(darkModeTogleEl, 'flex')
+            } else {
+                document.documentElement.classList.add('dark');
+                localStorage.setItem('dark-mode', 'true');


Suggested change

localStorage.setItem('dark-mode', 'true');

localStorage.setItem('dark-mode', JSON.stringify(true));

cherryramatisdev · 2024-07-22T21:53:38Z

src/js/dark-theme.js

+    }
+
+
+    if (localStorage.getItem('dark-mode') === 'false') {


Suggested change

if (localStorage.getItem('dark-mode') === 'false') {

if (!JSON.parse(localStorage.getItem('dark-mode'))) {

cherryramatisdev · 2024-07-22T21:54:15Z

src/js/dark-theme.js

@@ -0,0 +1,41 @@
+if (localStorage.getItem('dark-mode') === 'true') {


Suggested change

if (localStorage.getItem('dark-mode') === 'true') {

if (JSON.parse(localStorage.getItem('dark-mode'))) {

dgarcia360 · 2024-07-23T17:19:51Z

Thanks @danielhe4rt!

The theme currently uses Foundation Framework. Do we need to load Tailwind and have two CSS frameworks just for this feature?

Note that the theme already defines all colors in _colors.scss.

I was thinking a better approach would be to change SCSS variables to standard CSS variables and define two color schemas as follows:

:root {
  --color-black: #000;
  --color-white: #ffffff;

  --color-background-footer: var(--color-white);
  (...)
}

.dark-mode {

  --color-background-footer: var(--color-black);
  (...)
}

Similar to what you did on https://github.com/scylladb/sphinx-scylladb-theme/pull/1146/files#diff-a37591e286af1d6159d657820ec36c4a9df2696a4ac603893c33e5601a7a6c2c, but controlling and overriding all the colors from there, not just:

Then, we can load the dark theme class conditionally to the tag once we click the toggle button.

This way, we have control over colors in just one file, we don't have to edit every CSS class, and we don't need Tailwind at all.

annastuchlik · 2024-08-01T09:51:05Z

Hi @danielhe4rt,

Thanks for opening this PR. We definitely need a dark theme for the docs.
It looks good, but I have some concerns about the implementation. Whenever we add something to the Sphinx theme, we must make sure it will be maintainable, and I'm not sure the current implementation meets the requirement.

We should avoid having two different CSS frameworks in one Sphinx theme, so I'd rather not use Tailwind if possible.
The proposed approach involves updating multiple CSS class files, affecting maintainability. It would be better to use just one file.

In short, I fully agree with the approach suggested by @dgarcia360 above. Could you update this PR or open a new one to apply his suggestions?

danielhe4rt · 2024-08-02T18:07:40Z

Hey @dgarcia360 and @annastuchlik.

I have no problem to only use one framework into it. However IMHO it will be way easier to maintain a TailwindCSS project...

There's too much hardcoded width and heights in the current project in a way that in bigger screens it would be a problem. With TailwindCSS this wouldn't be an issue for each component.

But I'm not a Front-end Developer, I'm just a curious person with a developer stack and a dream LOL...

In each case, I'll try to implement the color scheme like @dgarcia360 suggested, but I still gonna run a fork for a project that I'm working on with @timkoopmans using TailwindCSS.

tzach · 2024-08-04T00:32:51Z

@kwiato please take a look

tzach · 2024-08-04T10:13:21Z

replaced by #1164

dgarcia360 · 2024-08-06T10:31:52Z

Hi @danielhe4rt

I have no problem to only use one framework into it. However IMHO it will be way easier to maintain a TailwindCSS project...

We can always switch frameworks if there are clear benefits. If you think it's a better approach, consider creating a new issue with a proposal. It would be helpful to see a comparison of pros and cons, as well as an outline of what changes would be needed to understand the impact.

There's too much hardcoded width and heights in the current project in a way that in bigger screens it would be a problem. With TailwindCSS this wouldn't be an issue for each component.

I'm not sure how this relates to the dark mode, could you please elaborate? Bigger screens can always be handled with media queries. Switching to TailwindCSS won't automatically fix having fixed widths and heights (it's only a wrapper on top of CSS), it will depend on the implementation.

In each case, I'll try to implement the color scheme like @dgarcia360 suggested, but I still gonna run a fork for a project that I'm working on with @timkoopmans using TailwindCSS.

Thanks! Let's continue with the revision in the next PR.

timkoopmans · 2024-08-08T07:46:08Z

Hi @dgarcia360

Thanks for the info - I didn't realize we were using foundation for the scylladb sphinx theme. FWIW I don't have a strong preference for any theme, be it tailwind, foundation or material (which seems to be scylladb cloud for example). I do think it would be a good idea to start aligning subdomains a little more in terms of theming, but will let the new UI/UX person (Rafal) take lead on that.

In the short term, if I wanted to introduce a simple card component e.g.

Does the current scylladb sphinx theme support native UI components of foundation e.g. https://get.foundation/sites/docs/card.html

Or do we have to roll our own CSS/HTML?

What would be the best way to work within the current implementation of our framework?

dgarcia360 · 2024-08-08T08:11:52Z

@timkoopmans consider using the topic box directive: https://sphinx-theme.scylladb.com/stable/examples/topic-box.html#topic-box

timkoopmans · 2024-08-08T09:46:21Z

I did see topic box. Question is do we implement full set of foundation UI elements or is it a subset?

Luisnadachi reviewed Jul 22, 2024

View reviewed changes

Daniel-Boll reviewed Jul 22, 2024

View reviewed changes

cherryramatisdev suggested changes Jul 22, 2024

View reviewed changes

danielhe4rt closed this Aug 3, 2024

danielhe4rt force-pushed the master branch from 957d580 to b05a118 Compare August 3, 2024 00:19

		@@ -1,4 +1,4 @@
		<div id="side-nav" class="side-nav custom-scroll-bar" data-closable data-toggler=".show">
		<div id="side-nav" class="side-nav @apply dark:bg-navigation-base custom-scroll-bar" data-closable data-toggler=".show">

	<div id="side-nav" class="side-nav @apply dark:bg-navigation-base custom-scroll-bar" data-closable data-toggler=".show">
	<div id="side-nav" class="side-nav dark:bg-navigation-base custom-scroll-bar" data-closable data-toggler=".show">

	localStorage.setItem('dark-mode', 'false');
	localStorage.setItem('dark-mode', JSON.stringify(false));

	localStorage.setItem('dark-mode', 'true');
	localStorage.setItem('dark-mode', JSON.stringify(true));

	if (localStorage.getItem('dark-mode') === 'false') {
	if (!JSON.parse(localStorage.getItem('dark-mode'))) {

		@@ -0,0 +1,41 @@
		if (localStorage.getItem('dark-mode') === 'true') {

	if (localStorage.getItem('dark-mode') === 'true') {
	if (JSON.parse(localStorage.getItem('dark-mode'))) {

feat: sphinx dark theme #1146

feat: sphinx dark theme #1146

Uh oh!

Conversation

danielhe4rt commented Jul 22, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Hey folks!

Motivation

PoC/Concept

Actual Implementation

Uh oh!

tzach commented Jul 22, 2024

Uh oh!

tzach commented Jul 22, 2024

Uh oh!

danielhe4rt commented Jul 22, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dgarcia360 commented Jul 23, 2024

Uh oh!

annastuchlik commented Aug 1, 2024

Uh oh!

danielhe4rt commented Aug 2, 2024

Uh oh!

tzach commented Aug 4, 2024

Uh oh!

tzach commented Aug 4, 2024

Uh oh!

dgarcia360 commented Aug 6, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

timkoopmans commented Aug 8, 2024

Uh oh!

dgarcia360 commented Aug 8, 2024

Uh oh!

timkoopmans commented Aug 8, 2024

Uh oh!

Uh oh!

danielhe4rt commented Jul 22, 2024 •

edited

Loading

dgarcia360 commented Aug 6, 2024 •

edited

Loading