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

add background color based on the OS mode #7117

Merged

Conversation

aileenvl
Copy link
Contributor

Description

There is no background applied to the body and it defaults to white with a function to detect the users os mode we can change that background before gradio loads. there might be a better way to implement but just a draft idea.

Closes: #5392

🎯 PRs Should Target Issues

Before your create a PR, please check to see if there is an existing issue for this change. If not, please create an issue before you create this PR, unless the fix is very small.

Not adhering to this guideline will result in the PR being closed.

Tests

  1. PRs will only be merged if tests pass on CI. To run the tests locally, please set up your Gradio environment locally and run the tests: bash scripts/run_all_tests.sh

  2. You may need to run the linters: bash scripts/format_backend.sh and bash scripts/format_frontend.sh

@gradio-pr-bot
Copy link
Contributor

gradio-pr-bot commented Jan 23, 2024

🪼 branch checks and previews

Name Status URL
Storybook ready! Storybook preview
Visual tests all good! Build review
🦄 Changes detected! Details

@gradio-pr-bot
Copy link
Contributor

gradio-pr-bot commented Jan 23, 2024

🦄 change detected

This Pull Request includes changes to the following packages.

Package Version
@gradio/app patch
gradio patch
  • Maintainers can select this checkbox to manually select packages to update.

With the following changelog entry.

add background color based on the OS mode

Maintainers or the PR author can modify the PR title to modify this entry.

Something isn't right?

  • Maintainers can change the version label to modify the version bump.
  • If the bot has failed to detect any changes, or if this pull request needs to update multiple packages to different versions or requires a more comprehensive changelog entry, maintainers can update the changelog file directly.

@pngwn
Copy link
Member

pngwn commented Jan 25, 2024

Hey @aileenvl! Thanks for this PR; this is a great idea!

I have just tweaked it a little so instead of using JS to set the color, we use static CSS instead. This way we can guarantee we never have any flashes by putting it right at the top of the head. This did require some changes to the build config but seems to be working now. I also added a default font until our custom fonts load in.

There is no flashing now and the font switch is pretty subtle.

Thanks again!

body {
background: white;
color: black;
font-family: Arial, Helvetica, 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.

Prevents the janky switch from a default serif to our sans-serif.

@@ -115,6 +115,7 @@ export default defineConfig(({ mode }) => {
} else if (
selector.indexOf(":root") > -1 ||
selector.indexOf("dark") > -1 ||
selector.indexOf("body") > -1 ||
Copy link
Member

Choose a reason for hiding this comment

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

We don't want to add a prefix to body selectors, that breaks them.

@aliabid94
Copy link
Collaborator

Is there any we we can use the theme variables, --body-background-fill --body-background-fill-dark, body-text-color, etc?

@pngwn
Copy link
Member

pngwn commented Jan 25, 2024

We can but we would have to define it right alongside the definition (which seems pointless to me at least) because the styles where we define those things haven't loaded yet, they get loaded in by the gradio app and this flash happens because of that delay.

@aliabid94
Copy link
Collaborator

aliabid94 commented Jan 25, 2024

but now there will be a flash if those colors differ, correct?
would it be overkill to pass those theme colors directly to the index.html template at render time so there's no flash at all?

@aliabid94
Copy link
Collaborator

aliabid94 commented Jan 25, 2024

e.g.

		<style>
			body {
				background: {{ config['theme-bg'] }};
				color: {{ config['theme-color'] }};
				font-family: Arial, Helvetica, sans-serif;
			}

			@media (prefers-color-scheme: dark) {
				body {
    		         		background: {{ config['theme-bg-dark'] }};
			        	color: {{ config['theme-color-dark'] }};
				}
			}
		</style>

@pngwn
Copy link
Member

pngwn commented Jan 25, 2024

but now there will be a flash if those colors differ, correct?

Yes, if they get out of sync.

would it be overkill to pass those theme colors directly to the index.html template at render time so there's no flash at all?

No, i think that makes perfect sense, would be ideal.

Will take a look!

@aliabid94
Copy link
Collaborator

I can take a crack at it if its late and you can review in the morning?

@pngwn
Copy link
Member

pngwn commented Jan 25, 2024

@aliabid94 Sounds good! Thansk!

@pngwn
Copy link
Member

pngwn commented Jan 25, 2024

Feel free to merge if you get it working and want to get the release out. The best way to test is just to add some network throttling with the network tab open and there should be no color changes. You'll know where the color flash should come in because you'll see the font change (it is subtle in this branch), it tends to happen around the same time.

@aliabid94
Copy link
Collaborator

I couldn't figure out how to pass the config values to the CSS, because:

		<style>
			body {
				background: {{ config['body_css']['body_background_fill'] }};
				color: {{ config['body_css']['body_text_color'] }};
				font-family: Arial, Helvetica, sans-serif;
			}

			@media (prefers-color-scheme: dark) {
				body {
					background: {{ config['body_css']['body_background_fill_dark'] }};
					color: {{ config['body_css']['body_text_color_dark'] }};
				}
			}
		</style>

would cause the compiler to fail parsing the CSS. Can't figure out how to embed the tempate tags in CSS since they're not quote enclosed

@pngwn
Copy link
Member

pngwn commented Jan 26, 2024

@aliabid94 which compiler? The vite build or the templating compiler?

@aliabid94
Copy link
Collaborator

vite

11:07:36 AM [vite-plugin-svelte] you are building for production but compilerOptions.dev is true, forcing it to false
vite v4.3.9 building for production...
✓ 2 modules transformed.
✓ built in 53ms
[vite:css] [postcss] /Users/aliabid/Desktop/gradio/js/app/index.html?html-proxy&inline-css&index=0.css:3:20: Unknown word
file: /Users/aliabid/Desktop/gradio/js/app/index.html?html-proxy&inline-css&index=0.css:3:20
error during build:
CssSyntaxError: [postcss] /Users/aliabid/Desktop/gradio/js/app/index.html?html-proxy&inline-css&index=0.css:3:20: Unknown word
    at Input.error (/Users/aliabid/Desktop/gradio/node_modules/.pnpm/postcss@8.4.27/node_modules/postcss/lib/input.js:106:16)
    at Parser.unknownWord (/Users/aliabid/Desktop/gradio/node_modules/.pnpm/postcss@8.4.27/node_modules/postcss/lib/parser.js:587:22)
    at Parser.other (/Users/aliabid/Desktop/gradio/node_modules/.pnpm/postcss@8.4.27/node_modules/postcss/lib/parser.js:429:12)
    at Parser.parse (/Users/aliabid/Desktop/gradio/node_modules/.pnpm/postcss@8.4.27/node_modules/postcss/lib/parser.js:464:16)
    at parse (/Users/aliabid/Desktop/gradio/node_modules/.pnpm/postcss@8.4.27/node_modules/postcss/lib/parse.js:11:12)
    at new LazyResult (/Users/aliabid/Desktop/gradio/node_modules/.pnpm/postcss@8.4.27/node_modules/postcss/lib/lazy-result.js:133:16)
    at Processor.process (/Users/aliabid/Desktop/gradio/node_modules/.pnpm/postcss@8.4.27/node_modules/postcss/lib/processor.js:53:14)
    at compileCSS (file:///Users/aliabid/Desktop/gradio/node_modules/.pnpm/vite@4.3.9_@types+node@20.3.2_less@4.2.0/node_modules/vite/dist/node/chunks/dep-e8f070e8.js:38481:63)
    at async Object.transform (file:///Users/aliabid/Desktop/gradio/node_modules/.pnpm/vite@4.3.9_@types+node@20.3.2_less@4.2.0/node_modules/vite/dist/node/chunks/dep-e8f070e8.js:37917:56)
    at async transform (file:///Users/aliabid/Desktop/gradio/node_modules/.pnpm/rollup@3.29.4/node_modules/rollup/dist/es/shared/node-entry.js:24449:16)
/Users/aliabid/Desktop/gradio/js/app:
 ERR_PNPM_RECURSIVE_RUN_FIRST_FAIL 

@aliabid94
Copy link
Collaborator

ready for review

@@ -93,6 +94,30 @@ def repl_func(match):

return f"{css_code}\n{dark_css_code}"

def _get_computed_value(self, property: str, depth=0) -> str:
MAX_DEPTH = 100
if depth > MAX_DEPTH:
Copy link
Collaborator

Choose a reason for hiding this comment

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

don't want to raise an exception in case there were "broken" themes with circular references before. They didn't error out before and don't want a breaking change.

@abidlabs
Copy link
Member

Tested locally and works great! Thanks everyone ❤️

Copy link
Member

@pngwn pngwn left a comment

Choose a reason for hiding this comment

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

Looks (and works) great! Thanks @aileenvl @aliabid94!

@abidlabs
Copy link
Member

@aliabid94 re: the tests, I ran them all locally and they passed, so I'll go ahead and merge this in.

cc @gary149

@abidlabs abidlabs merged commit 24157a3 into gradio-app:main Jan 30, 2024
8 of 13 checks passed
@pngwn pngwn mentioned this pull request Jan 30, 2024
@gary149
Copy link
Contributor

gary149 commented Mar 7, 2024

fyi there's still a white background while loading on Spaces in dark mode:

image

@abidlabs
Copy link
Member

abidlabs commented Mar 7, 2024

Yup seeing it too -- will reopen the issue

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.

White background appears while a Gradio app is loading even in dark mode
7 participants