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

nginx-directory: add light/dark style #1394

Closed
wants to merge 1 commit into from

Conversation

asimpson
Copy link

@asimpson asimpson commented Feb 7, 2025

Adds light/dark theme support to the nginx directory index.html page.

Adds some basic styling as well. Including making the help "tooltip" feature bigger since it's difficult to mouse over in its current size.

Dark mode:
image

Light mode:
image

@asimpson asimpson requested a review from iainlane February 7, 2025 21:04
Copy link
Member

@sarahzinger sarahzinger left a comment

Choose a reason for hiding this comment

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

🌔

Copy link
Member

@iainlane iainlane left a comment

Choose a reason for hiding this comment

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

cheers! i think there's scope for visual improvement here... just had some questions to get the ball rolling and I'll ask for more input

</ul>
</body>

</html>
Copy link
Member

Choose a reason for hiding this comment

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

Can you fix the missing trailing newline here?

}

body {
font-family: -apple-system, BlinkMacSystemFont, "Segoe UI", Roboto, sans-serif;
Copy link
Member

Choose a reason for hiding this comment

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

perhaps we could borrow Tailwind's default?

Suggested change
font-family: -apple-system, BlinkMacSystemFont, "Segoe UI", Roboto, sans-serif;
font-family: ui-sans-serif, system-ui, sans-serif, 'Apple Color Emoji', 'Segoe UI Emoji', 'Segoe UI Symbol', 'Noto Color Emoji';

}

ul {
list-style: none;
Copy link
Member

Choose a reason for hiding this comment

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

Can you say more about why you chose this?

display: inline-block;
padding: 8px;
color: #424242;
text-decoration: none;
Copy link
Member

Choose a reason for hiding this comment

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

And this one... to me (just looked at the screenshots) it seems less obvious that it's a list of links now

font-family: -apple-system, BlinkMacSystemFont, "Segoe UI", Roboto, sans-serif;
margin: 0;
padding: 24px;
color: #424242;
Copy link
Member

Choose a reason for hiding this comment

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

could we consider using CSS variables to make the theme easier to grok / modify? could be a way to do the light/dark theme too.

padding: 4px 12px;
}

@media (prefers-color-scheme: dark) {
Copy link
Member

Choose a reason for hiding this comment

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

as I understand this, it will always sync with the "system" setting, right?

no opinion on that right now - just wanted to call it out and see what others think. might be that if we did this we'd like a toggle that saves the preference in local storage, because not all folks will like that.

@iainlane iainlane requested a review from Duologic February 10, 2025 10:02
Copy link
Member

@Duologic Duologic left a comment

Choose a reason for hiding this comment

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

You have to consider that we stuff this file into a ConfigMap, the size matters (<1mb), I bet it does fit, just raising awareness.

@Duologic
Copy link
Member

I'd personally argue not to style this page, the point is to keep the page dead simple. Adding styles opens the door for all kinds of personal preferences.

There are plugins that can configure any page to dark-style: https://darkreader.org/ and there is also some browsers supporting user-stylesheets, if you must I'd suggest looking at that.

@asimpson
Copy link
Author

You have to consider that we stuff this file into a ConfigMap, the size matters (<1mb), I bet it does fit, just raising awareness.

Yep, that was one of my main considerations in doing this as a few lines of inline CSS (total page weighs in around 2.9K) instead of anything heavier.

I'd personally argue not to style this page, the point is to keep the page dead simple. Adding styles opens the door for all kinds of personal preferences.

Fair enough. I'll close the PR, thanks for the review 👍

@asimpson asimpson closed this Feb 10, 2025
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.

4 participants