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 CSS to be loaded before replacing the Body #614

Merged
merged 9 commits into from Jul 29, 2022

Conversation

manuelpuyol
Copy link
Contributor

Related to #613

As suggested in #613 (comment), we can listen for load and error events in stylesheets to make sure they are loaded before continuing a render.

Here I'm updating how PageRenderer adds new stylesheets to the document head, adding listeners to those events and waiting them to fire before replacing the body.

src/util.ts Outdated Show resolved Hide resolved
@dhh
Copy link
Member

dhh commented Jun 29, 2022

What would the failure state be if those assets fail to load (bad CDN or whatever)? How long would we wait for them to load? Do we need a timeout?

@manuelpuyol
Copy link
Contributor Author

What would the failure state be if those assets fail to load (bad CDN or whatever)? How long would we wait for them to load? Do we need a timeout?

good shout, I've added a timeout as @seanpdoyle suggested

Co-authored-by: Anthony Ricaud <anthony@ricaud.me>
@manuelpuyol
Copy link
Contributor Author

@seanpdoyle @rik what do you think of the changes now? I've implemented your suggestions :)

@dhh dhh added this to the 7.2.0 milestone Jul 16, 2022
@dhh
Copy link
Member

dhh commented Jul 18, 2022

Do we have adequate test coverage for this?

@manuelpuyol
Copy link
Contributor Author

Do we have adequate test coverage for this?

added tests in 3171b0a

@manuelpuyol
Copy link
Contributor Author

@dhh this should be ready for some 👀

@dhh dhh merged commit 18d088b into hotwired:main Jul 29, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants