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.

Sign up for GitHub

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

Jump to bottom

Docs dark theme rebased #1210

Merged
merged 8 commits into from
Sep 20, 2024
Merged

Docs dark theme rebased #1210

merged 8 commits into from
Sep 20, 2024

Conversation

kwiato
Copy link
Contributor

@kwiato kwiato commented Sep 17, 2024

No description provided.

@dgarcia360 dgarcia360 mentioned this pull request Sep 17, 2024
Closed
@dgarcia360
Copy link
Collaborator

Some comments:

  • Icon not rendering as expected:
image
  • Icon not aligned:
image
  • Icon significant smaller than the previous version -> is it intended?
image
  • Review default label color in light mode:
image
  • Review announcement button color in dark mode:
image
  • Review icons spacing:
image
  • The "User mailing list" icon should not render, we removed it recently:
image
  • Dropdown arrows are smaller -> is it intended?
image
  • Test do not pass. I'll send the fix.

dgarcia360
dgarcia360 reviewed Sep 17, 2024
View reviewed changes
src/css/components/_icons.scss
filter: brightness(0) saturate(100%) invert(100%) sepia(4%) saturate(10%) hue-rotate(140deg) brightness(106%) contrast(100%);
}
}

.scylla-icon {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Looks safe to remove part of these icons if we do not use them anymore and they are defined in the new icons font.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wanted to wait untill tested in other docs

dgarcia360
dgarcia360 reviewed Sep 17, 2024
View reviewed changes
src/css/base/variables/_colors.scss
@@ -1,102 +1,121 @@
/* Color names from https://chir.ag/projects/name-that-color */
:root {
Copy link
Collaborator

Choose a reason for hiding this comment

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

We are currently defining colors in two separate files: base/_variables.scss and variables/_colors.scss. To improve maintainability, we should consolidate these into a single file. Additionally, we should remove any unused variables and colors.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agree, just want to make sure I don't break anything right now.

dgarcia360
dgarcia360 reviewed Sep 17, 2024
View reviewed changes
src/css/main.scss Outdated
@@ -1,4 +1,6 @@
@import "~foundation-sites/dist/css/foundation.min.css";
@import "../../node_modules/foundation-sites/dist/css/foundation.min.css";
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why is this change necessary?

@kwiato kwiato force-pushed the docs-dark-theme-rebased branch from 5e620c0 to 358366a Compare September 19, 2024 14:02
@dgarcia360
Copy link
Collaborator

dgarcia360 commented Sep 20, 2024 •
edited
Loading

Feedback:

  • The "x" button to close the announcement banner has disappeared:
image

  • When navigating between pages in dark mode, the light mode briefly flashes for a few milliseconds. We can work on this as part of a separate PR. To evaluate if it's a blocker for releasing.

  • Review the footer in responsive mode:

    image

    Before:

    image

  • It's important to clean up the codebase before the release:

    We are currently defining colors in two separate files: base/_variables.scss and variables/_colors.scss. To improve maintainability, we should consolidate these into a single file. Additionally, we should remove any unused variables and colors.

    Styles are only defined in this project for all projects, so it should be safe to update them as long as we test them. If you'd prefer not to address this in the current PR, we can handle it as part of a separate one.

@dgarcia360
Copy link
Collaborator

Fixed tests: 70811e4

@dgarcia360
Copy link
Collaborator

Icons that are not using the new icons library:

  • Left sidebar expand button (fas fa-bars):
    image

  • Tables checks (fa fa-check) and cross marks (fa fa-times)
    image

  • Hero box & topic documentation examples: /examples/hero-box & /examples/topic-box

This was referenced Sep 20, 2024
Closed
Merged
@kwiato kwiato force-pushed the docs-dark-theme-rebased branch from 70811e4 to 6a2a3ef Compare September 20, 2024 11:11
@kwiato kwiato force-pushed the docs-dark-theme-rebased branch from 6a2a3ef to 37f2217 Compare September 20, 2024 11:13
@dgarcia360 dgarcia360 mentioned this pull request Sep 20, 2024
Closed
@dgarcia360
Copy link
Collaborator

LGTM. Let's wait for @annastuchlik review before merging.

New issues derived from the changes in this PR:

To decide if we want to include them in the next release.

@dgarcia360 dgarcia360 self-requested a review September 20, 2024 11:36
annastuchlik
annastuchlik approved these changes Sep 20, 2024
View reviewed changes
Copy link
Collaborator

@annastuchlik annastuchlik 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 :)

@annastuchlik
Copy link
Collaborator

@kwiato @dgarcia360 Thank you :) We can now merge and move on :)

@dgarcia360 dgarcia360 merged commit 566db08 into scylladb:master Sep 20, 2024
5 checks passed
@tzach
Copy link
Collaborator

tzach commented Sep 24, 2024

@dgarcia360 why cant I see dark node on the Theme site? https://sphinx-theme.scylladb.com/stable/

@annastuchlik
Copy link
Collaborator

@tzach We need to merge this PR: #1216
We'll do it today.
The new version contains major changes, so we have do release the new theme with separate PRs.

@annastuchlik
Copy link
Collaborator

What I mean is that Dark Theme is part of the new theme that needs to be released yet.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@annastuchlik annastuchlik annastuchlik approved these changes

@dgarcia360 dgarcia360 dgarcia360 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@kwiato @dgarcia360 @annastuchlik @tzach @danielhe4rt