sharetribe / ftw-daily

Sharetribe Flex Template for Web
Other
102 stars 944 forks source link

The conflict between `setPageScrollPosition` and `Loadable Component` #1478

Closed anh-dinh-jh closed 2 years ago

anh-dinh-jh commented 3 years ago

Describe the bug

The new feature - Loadable Components of ftw-daily v8.0.0 (#1414) has a conflict with the old function - setPageScrollPosition in Route.js. With the new "lazy" server-side rendering, this line doesn't work anymore as it can't find the correct element anymore, so the scrolling behavior is not triggered correctly. It still works perfectly if it's on the same page but it doesn't work if you are on another page.

const el = document.querySelector(location.hash);

To Reproduce

  1. Set an id="howitworks" to the list item - a parent of the component SectionHowItWorks
  2. Create a nav link - on the topbar that will navigate the user to LandingPage with hash: #howitworks
  3. Click the button. If the current page is also LandingPage, it scrolls to the section How It Works. Otherwise, it doesn't.

Expected behavior

Possible solutions

Gnito commented 3 years ago

Thanks for reporting! We'll look into this.

Gnito commented 2 years ago

@anh-dinh-jh Sorry for the delayed answer. This has been a bit out of focus due to our development cycles.

Anyway, we decided that this belongs to a "won't fix" category. Smooth scrolling works when navigating within the current page - which is the main use case for this smooth feature in templates.

To make this work correctly with in-app navigation between pages, the feature should wait for correct code-chunk and loadData fetch to finish.

That complicates codebase - so, for template purpose that doesn't have good ROI.

If feature is needed on customization, my initial suggestion is to move the scroll-behaviour from setPageScrollPosition to callLoadData function. That way, scrollIntoView can be called in then method for pages that need loadData call and potentially some timeout could be used for static pages (pages that don't have needs for data loading).

Related to this update to code comment https://github.com/sharetribe/ftw-daily/pull/1484