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

Update <html> element attributes when navigating #553

Merged
merged 12 commits into from
Mar 31, 2022
Merged

Update <html> element attributes when navigating #553

merged 12 commits into from
Mar 31, 2022

Conversation

manuelpuyol
Copy link
Contributor

Turbo only replaces the <body> and updates the <head> when navigating, ignoring possible changes to the <html> element, like classes or data-* attributes.
With this change, Turbo will update the <html> when navigating through pages.

@dhh
Copy link
Member

dhh commented Mar 30, 2022

Bunch of failures. But seem unlikely that they're directly related to this.

@manuelpuyol
Copy link
Contributor Author

👋 @dhh this should be ready now! Mind approving the workflow?

@dhh
Copy link
Member

dhh commented Mar 31, 2022

There's just one failure now, but that's definitely not related to this change, so good to go:

Screen Shot 2022-03-31 at 10 48 25

(But would be great to have a look at this too, so we can have a clean build!)

@dhh dhh merged commit da35d5b into hotwired:main Mar 31, 2022
@manuelpuyol manuelpuyol deleted the update-html-attributes branch March 31, 2022 14:51
@packagethief
Copy link
Contributor

I'd argue that not changing the document element during navigation is a feature. At Basecamp we use <html> as a permanent element to hold state that we don't want to change between pages.

It's useful for state that can't be obtained on the server and that won't change during a session. For example, data about the current theme, client capabilities, client permissions, or other client preferences that only need to be obtained once.

@manuelpuyol, was this motivated by a particular use case?

@dhh
Copy link
Member

dhh commented Apr 30, 2022

Yeah, looking at how we use HTML in Basecamp, this is going to be a regression. Can't ship this as is. Best we can do if we actually want to change this is to put it behind a configuration option.

@manuelpuyol
Copy link
Contributor Author

manuelpuyol commented May 28, 2022

@manuelpuyol, was this motivated by a particular use case?

Sorry for the delay!

We also set some state data in the <html>, which may change when navigating pages. So pages that were setting new attributes or even classes weren't working since the new <html> was discarded

@packagethief
Copy link
Contributor

So pages that were setting new attributes or even classes weren't working since the new was discarded

I see, thanks for the explanation @manuelpuyol!

It's intentional that we're only replacing the <body> element, and it's not something we can change without ceremony. We're calling out its persistence explicitly in the documentation:

During rendering, Turbo Drive replaces the current <body> element outright and merges the contents of the <head> element. The JavaScript window and document objects, and the <html> element, persist from one rendering to the next.

https://turbo.hotwired.dev/handbook/introduction#turbo-drive%3A-navigate-within-a-persistent-process

If we did decide to change the behavior I think it would need to be more nuanced, such that changes to the document element are merged, as is done for <head>.

In any case, I think we should revert on the grounds that this is a breaking API change.

@dhh
Copy link
Member

dhh commented Jun 2, 2022

Concur. I missed that originally. @manuelpuyol can you do a revert PR? Then we can give it another try behind a configuration option or something. But we can't break the API as-is.

@manuelpuyol
Copy link
Contributor Author

Working on a revert here #596
I'm gonna keep the test setup changes to use localstorage though, I think they are still valuable :)

dhh pushed a commit that referenced this pull request Jun 3, 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.

3 participants