Refactor the way scroll is saved and restored to avoid premature restoration

[Intrepidd]

I realised #40 had a side effect where scroll would jump to 0 before each visit.

This seems to be because we listen on turbo:click to set the scroll position to 0 and we restore it in a turbo:before-visit event.

This looked a bit weird to me, I tried coming up with my own implementation that worked for me in my use cases, but this may be naive and I might be missing some crucial context.

Basically, I chose to store the scroll position only just before a reload is requested, and to restore and reset after a load.
This ensures we always save the position at the right time, negating the need for debounced scroll position etc.

[mikker]

Sounds reasonable and very nice to remove the flaky debounce stuff 👏

[elik-ru]

I would delete stored position right after it was restored.
This will ensure it won't try scroll on next visits.

[Intrepidd]

I would delete stored position right after it was restored.

This is what I'm doing here, isn't it ?

[Intrepidd]

Or do you mean actually destroying it rather than setting it to zero ?

[elik-ru]

Or do you mean actually destroying it rather than setting it to zero ?

Yes, I mean completely destroy it.

When restoring:

1. if there is no previously stored position - don't touch scroll at all.
2. Otherwise scroll to stored position
3. Destroy stored value.

[kirillplatonov]

Looks like a nice simplification! And I agree with @elik-ru suggestion, skipping scroll restoration when there's no stored position is better.

[Intrepidd]

Makes a lot of sense! Just pushed a commit in this direction.

[kirillplatonov]

Thanks @Intrepidd 👏

[Intrepidd]

@kirillplatonov any way we can get a release pushed ? Thanks !

[kirillplatonov]

Published v1.3.1

[mikker]

This isn't working for me anymore. Anyone else having the same experience?

[Intrepidd]

This isn't working for me anymore. Anyone else having the same experience?

What are you experiencing ? No issues on my end and no more intempestive scroll reset.

[mikker]

It doesn't seem to save the scroll position when scrolling. If I set it manually, it does restore correctly though.
Edit Huh, now it's working. Will investigate a bit.

[mikker]

I see the problem now - it's because Tailwind builds a new build when I save the view. So it goes

1. Save index.html.slim
2. Livereload sees the change, saves scroll, fires a reload
3. Page loads but doesn't make it to the scroll in time for…
4. Tailwind builds application.css
5. Livereload sees the change, fires a reload
6. Page loads but scroll wasn't saved

Not sure how to fix this yet. Any thoughts?

[Intrepidd]

Ah yes ! Actually I have this in my config since I started using this gem :

# Only rely on the builds folder for reloading to avoid double reloads
config.hotwire_livereload.listen_paths = [Rails.root.join("app/assets/builds")]

I rely on only one the builds folder, which is updated when my view changes, through tailwind :)

[elik-ru]

I see the problem now - it's because Tailwind builds a new build when I save the view. So it goes

1. Save index.html.slim
2. Tailwind builds application.css

Not sure how to fix this yet. Any thoughts?
I would say that changing just view should not rebuild css. Probably something can be adjusted here?

[Intrepidd]

I would say that changing just view should not rebuild css. Probably something can be adjusted here?

This is the way tailwind works, it listens to changes in your HTML / rb files to detect tailwind classes used so it can generate its CSS with only the classes you use.

[mikker]

Only listening to Tailwind, of course! That'll fix it. Might be a helpful thing to add as a note to the Readme?

[kirillplatonov]

Ah yes ! Actually I have this in my config since I started using this gem :

# Only rely on the builds folder for reloading to avoid double reloads
config.hotwire_livereload.listen_paths = [Rails.root.join("app/assets/builds")]

I rely on only one the builds folder, which is updated when my view changes, through tailwind :)

This looks like a great solution for Tailwind users! Worth mentioning in the Readme.

[kirillplatonov]

Fully overriding default listen paths is probably not necessary. In #52 I added ability to skip only few default folders that are conflicting with Tailwind:

  # ...
  config.hotwire_livereload.skip_listen_paths << Rails.root.join("app/helpers")
  config.hotwire_livereload.skip_listen_paths << Rails.root.join("app/javascript")
  config.hotwire_livereload.skip_listen_paths << Rails.root.join("app/views")