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

Fix wasm_proxied_mount_css to not reuse an existing style element #7709

Merged
merged 2 commits into from Mar 18, 2024

Conversation

whitphx
Copy link
Member

@whitphx whitphx commented Mar 15, 2024

Description

Fixes #7694

See this comment about this PR -> #7709 (comment)

… replace it for theme updates upon rerunning a new app
@gradio-pr-bot
Copy link
Contributor

gradio-pr-bot commented Mar 15, 2024

🪼 branch checks and previews

Name Status URL
Spaces ready! Spaces preview
Website building...
Storybook failed! Details
🦄 Changes detected! Details

Install Gradio from this PR

pip install https://gradio-builds.s3.amazonaws.com/077218139b4ac1a947dad385e3cf227215425b9c/gradio-4.21.0-py3-none-any.whl

Install Gradio Python Client from this PR

pip install "gradio-client @ git+https://github.com/gradio-app/gradio@077218139b4ac1a947dad385e3cf227215425b9c#subdirectory=client/python"

@gradio-pr-bot
Copy link
Contributor

gradio-pr-bot commented Mar 15, 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.

Fix wasm_proxied_mount_css to not reuse an existing style element

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.

`style[data-wasm-path='${url_string}']`
);
if (existing_link) return;
Copy link
Member Author

Choose a reason for hiding this comment

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

This was implemented after the non-Wasm ver. which skips mounting the CSS if there is always a mounted element as below.

gradio/js/app/src/css.ts

Lines 17 to 19 in 28342a2

const existing_link = document.querySelector(`link[href='${_url}']`);
if (existing_link) return Promise.resolve();

However, Lite is different from the normal one in that the app can be executed repeatedly and the frontend can be refreshed many times, so this design was not correct and the requested CSS must be mounted, replacing the old one.

@whitphx
Copy link
Member Author

whitphx commented Mar 15, 2024

Even this change, the playground still has a problem that the background is not styled correctly.
Will fix it as well.

Playground: CleanShot 2024-03-15 at 15 48 58@2x
Expected:
CleanShot 2024-03-15 at 15 52 10@2x

@whitphx
Copy link
Member Author

whitphx commented Mar 15, 2024

The reason why the background theme is not applied is this:

if (window.__gradio_mode__ !== "website") {
active_theme_mode = handle_darkmode(wrapper);
}

Because handle_darkmode() is not called here due to the if-condition, darkmode() is not called finally, then the background-update code below is not executed.
bg_element.style.background = "var(--body-background-fill)";

→What about enforcing the light theme instead of skipping applying the theme when window.__gradio_mode__ === "website"? Created a separate PR: #7710

Copy link
Collaborator

@aliabd aliabd left a comment

Choose a reason for hiding this comment

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

Thanks @whitphx everything lgtm.

there's a funny effect on playground where the theme also affects the code component.

Screen Shot 2024-03-18 at 1 31 25 AM

but i actually think that's fine

@whitphx
Copy link
Member Author

whitphx commented Mar 18, 2024

@aliabd Thanks!
Let's discuss about the playground theming issue if you find it not good later 👍

@abidlabs
Copy link
Member

Merging as well so that we can do a release soon. Thanks again @whitphx!

@abidlabs abidlabs merged commit f67759d into main Mar 18, 2024
8 of 9 checks passed
@abidlabs abidlabs deleted the update-css-after-reload branch March 18, 2024 14:30
@pngwn pngwn mentioned this pull request Mar 18, 2024
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.

Does GradioLite support Gradio custom theme?
4 participants