Skip to content

Themes are broken the first time a page is loaded #573

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

Closed
Michael-F-Bryan opened this issue Jan 23, 2018 · 4 comments
Closed

Themes are broken the first time a page is loaded #573

Michael-F-Bryan opened this issue Jan 23, 2018 · 4 comments

Comments

@Michael-F-Bryan
Copy link
Contributor

I noticed that on master when you open up a book you've previously visited it'll be rendered without any CSS styling. This fixes itself when you change the themes and will never show itself again for that domain.

I've seen something similar in the past where we'd changed the theme name being stored in local storage (because of a typo it went from light to "light", or vice versa). I believe the underlying issue is that when loading the theme we don't properly handle the case where the theme specified in local storage doesn't exist.

I noticed this when rendering The Book as part of my final checks before pushing the 0.1 release. Unfortunately it blocks the release because styling will be broken for every single person who visits The Book (or any other document generated by mdbook) for the first time after this release and has been there before.

@sorin-davidoi we may need to have a look at #550, possibly reverting it if there isn't an easy fix.

@sorin-davidoi
Copy link
Contributor

sorin-davidoi commented Jan 23, 2018

I can't seem to be able to replicate this.

Yes, I can. store.js stored all it's values wrapped in quotes, just like you said...

@sorin-davidoi
Copy link
Contributor

I believe the underlying issue is that when loading the theme we don't properly handle the case where the theme specified in local storage doesn't exist.

Don't think that has changed in #550 - there is no validation now and there was no validation before as far as I can tell.

@Michael-F-Bryan
Copy link
Contributor Author

Michael-F-Bryan commented Jan 23, 2018

When we had this issue a while back it was something to do with how the index.hbs template was being rendered. Chances are we may have accidentally reverted that original fix when updating the templates, so me mentioning #550 was probably a red herring.

EDIT: Ignore that. store.js probably stores keys as JSON so you can store more complex objects, hence the quotes. To avoid breaking themes for everyone who's previously visited The Book, we should probably try to do the same thing.

We may want to insert a check in book.js to make sure local storage gives us a valid theme, even if there wasn't one before #550. That way we can fall back to the default theme if we try to load an invalid one for whatever reason.

@Michael-F-Bryan
Copy link
Contributor Author

Michael-F-Bryan commented Jan 23, 2018

Fixed in #574 🎉

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

No branches or pull requests

2 participants