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

Add Dark Theme #581

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Add Dark Theme #581

wants to merge 1 commit into from

Conversation

dasGoogle
Copy link
Contributor

@dasGoogle dasGoogle commented Jul 29, 2024 •
edited
Loading

Keep in mind to update bootstrap (you may need to manually delete some files so that the install works).

Closes #502

image

image

Changes small parts of the light theme:

image

@jeriox jeriox requested a review from SilvanVerhoeven July 29, 2024 19:04
@SilvanVerhoeven
Copy link
Collaborator

Keep in mind to update bootstrap (you may need to manually delete some files so that the install works).

Something I saw after I deleted the files manually: The script has an update (-u) and remove -r flag, which removes the previously installed version

SilvanVerhoeven

This comment was marked as duplicate.

Sign in to view

SilvanVerhoeven

This comment was marked as duplicate.

Sign in to view

SilvanVerhoeven

This comment was marked as duplicate.

Sign in to view

SilvanVerhoeven

This comment was marked as duplicate.

Sign in to view

SilvanVerhoeven

This comment was marked as duplicate.

Sign in to view

SilvanVerhoeven

This comment was marked as duplicate.

Sign in to view

SilvanVerhoeven

This comment was marked as duplicate.

Sign in to view

SilvanVerhoeven

This comment was marked as duplicate.

Sign in to view

SilvanVerhoeven

This comment was marked as duplicate.

Sign in to view

SilvanVerhoeven

This comment was marked as duplicate.

Sign in to view

SilvanVerhoeven

This comment was marked as duplicate.

Sign in to view

@SilvanVerhoeven

This comment was marked as duplicate.

Sign in to view
SilvanVerhoeven
SilvanVerhoeven requested changes Jul 29, 2024
View reviewed changes
myhpi/search/templates/search/search_result.html
@@ -9,7 +9,7 @@ <h5 class="text-break">{{ result.title }}</h5>
{{ result.specific.date|date:"SHORT_DATE_FORMAT" }}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Adding comments to random lines to make them resolvable one by one

Is it possible to remove the border (or make it same color as the highlight) on hover?

For that to work, we might need a more significant contrast between the hover highlight and the background color

This design:
grafik

Old design:
grafik

myhpi/search/templates/search/search_result.html
@@ -9,7 +9,7 @@ <h5 class="text-break">{{ result.title }}</h5>
{{ result.specific.date|date:"SHORT_DATE_FORMAT" }}
</time>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is it intentionally that the divider in the user menu seems to be gone?

Old menu for comparison:
grafik

myhpi/search/templates/search/search_result.html
@@ -9,7 +9,7 @@ <h5 class="text-break">{{ result.title }}</h5>
{{ result.specific.date|date:"SHORT_DATE_FORMAT" }}
</time>
</hgroup>
Copy link
Collaborator

Choose a reason for hiding this comment

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

The mobile menu needs its background back ^^

grafik

myhpi/search/templates/search/search_result.html
@@ -9,7 +9,7 @@ <h5 class="text-break">{{ result.title }}</h5>
{{ result.specific.date|date:"SHORT_DATE_FORMAT" }}
</time>
</hgroup>
<span class="text-dark">
<span class="search-preview">
{% if result.search_description %}
Copy link
Collaborator

Choose a reason for hiding this comment

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

The border at the bottom of the menu seems to be gone, which sometimes makes it hard to differentiate it from the background:

grafik

This is the old design:

grafik

myhpi/search/templates/search/search_result.html
@@ -9,7 +9,7 @@ <h5 class="text-break">{{ result.title }}</h5>
{{ result.specific.date|date:"SHORT_DATE_FORMAT" }}
</time>
</hgroup>
<span class="text-dark">
<span class="search-preview">
{% if result.search_description %}
{{ result.search_description }}
Copy link
Collaborator

Choose a reason for hiding this comment

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

The input elements now seems to be gray by default, which looks like they would be disabled. Could we make them white again?

grafik

grafik

myhpi/static/scss/footer.scss
@@ -1,5 +1,5 @@
.footer-myhpi {
background-color: var(--bs-gray-700);
background-color: var(--bs-gray-800);

Copy link
Collaborator

Choose a reason for hiding this comment

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

Is there a way to increase the contrast? Firefox throws a contrast warning

grafik

myhpi/static/scss/footer.scss
@@ -1,5 +1,5 @@
.footer-myhpi {
background-color: var(--bs-gray-700);
background-color: var(--bs-gray-800);

.page-content {
Copy link
Collaborator

Choose a reason for hiding this comment

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

The border between top bar and content is not visible any more. For better distinction, could we introduce it again?

This is how it looked:

grafik

myhpi/static/scss/footer.scss
@@ -1,5 +1,5 @@
.footer-myhpi {
background-color: var(--bs-gray-700);
background-color: var(--bs-gray-800);

.page-content {
padding-top: 1.5rem;
Copy link
Collaborator

Choose a reason for hiding this comment

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

A difference between the menu and background color would be helpful to differentiate both better (now they flow into each other):

grafik

Before:

grafik

Especially helpful in the dark mode:

grafik

myhpi/static/scss/myHPI.scss
@@ -5,20 +5,25 @@
$primary: #F5631E;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Making the breadcrumbs gray again could help differentiating them from the page content:

grafik

Before:
grafik

myhpi/static/scss/myHPI.scss
@@ -5,20 +5,25 @@
$primary: #F5631E;
$secondary: #F5631E;
Copy link
Collaborator

Choose a reason for hiding this comment

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

In [myhpi/core/templates/core/sidebar.html](https://github.com/fsr-de/myHPI/pull/525/files/bc3f7996429831f3857afe63e7b344cfd023d219#diff-8c66ac3d840d267494b184c335c50d73e56b70724f454183f3af2b86242632a7), there is the following comment:

<!-- Bootstrap 5.2 supports responsive offcanvas. This duplication is a workaround and should be removed after the upgrade -->

Could be addressed now, since we updated to 5.3

SilvanVerhoeven
SilvanVerhoeven reviewed Jul 29, 2024
View reviewed changes
myhpi/static/scss/myHPI.scss
$gray-600: #999;
$gray-700: #777;
$gray-800: #555;
$gray-900: #333;

Copy link
Collaborator

Choose a reason for hiding this comment

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

Is it wanted that there is no difference between the webpage background and the content background? The orange bar at the top and the footer at the bottom look a little bit lost to me without the content background

grafik

This might also fix the lack of contrast between table cells and content background:

grafik

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

@SilvanVerhoeven SilvanVerhoeven SilvanVerhoeven requested changes

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add dark mode
2 participants
@dasGoogle @SilvanVerhoeven