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

Wait for up to 1.5 seconds for external CSS to load #4335

Closed
wants to merge 2 commits into from

Conversation

akx
Copy link
Contributor

@akx akx commented May 25, 2023

Description

Checklist:

  • I have performed a self-review of my own code
  • I have added a short summary of my change to the CHANGELOG.md
  • My code follows the style guidelines of this project
  • I have commented my code in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes

akx added 2 commits May 25, 2023 10:12
It was never anything but `document.head`, and besides the `existing_link` check within the function didn't use it as scope anyway
@gradio-pr-bot
Copy link
Collaborator

All the demos for this PR have been deployed at https://huggingface.co/spaces/gradio-pr-deploys/pr-4335-all-demos

@pngwn
Copy link
Member

pngwn commented May 25, 2023

This isn't the right solution for offline loading and creates other possible issues on slow connections. We'll solve this properly in the near future.

@pngwn pngwn closed this May 25, 2023
@akx
Copy link
Contributor Author

akx commented May 25, 2023

@pngwn This would fix problems users are having in the meantime though. You could freely revert this when you implement the better solution ☺

@pngwn
Copy link
Member

pngwn commented May 29, 2023

Unfortunately it would also create problems that don't currently exist. So it isn't a viable approach.

@akx
Copy link
Contributor Author

akx commented May 29, 2023

@pngwn I'm curious – can you elaborate? What external CSS is critical enough to warrant waiting for? 🤔

@pngwn
Copy link
Member

pngwn commented May 30, 2023

If we don't load the CSS then the UI is basically unusable.While fonts aren't critical, other CSS files are, and this function is used to load many different kinds of CSS, not just fonts.

This change would potentially give users on slow connections a broken UI which isn't acceptable since Gradio is primarily a UI tool. While this does solve the offline usecase, that is a very niche usecase, whereas slow connections happen for a variety of reasons from poor infrastructure to poor mobile signal (and many reason in between). Creating a worse experience for those users in order to enable offline usage of gradio (which has been requested a few times but only by a handful of users), is not a good trade-off for the library.

We will fix this soon but not at the expense of other users' experience.

@akx
Copy link
Contributor Author

akx commented May 30, 2023

If we don't load the CSS then the UI is basically unusable.While fonts aren't critical, other CSS files are, and this function is used to load many different kinds of CSS, not just fonts.

Right, which is why this PR only adds the timeout to CSS that is loaded from an absolute source -- though that could also be changed to something like url.includes('googlefonts') ? 1500 : undefined :)

@akx akx mentioned this pull request Jun 2, 2023
@akx akx deleted the css-load-timeout branch June 27, 2023 10:38
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.

3 participants