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

Auto detect fragment in URL and jump to element #1118

Merged
merged 8 commits into from
Aug 2, 2022

Conversation

kennethnym
Copy link
Member

@kennethnym kennethnym commented Jul 6, 2022

Description

This PR fixes the issue where the page ignores URL fragments and doesn't scroll into the corresponding view automatically upon load.

I created a reusable hook called useAnchor that can be plugged into any page/component that require the functionality. Ideally, this can be done at the top level (i.e. <App />) so that all pages get this functionality by default.

The way I implemented it currently is a bit hacky. Due to components not actually mounted onto the DOM when useEffect is called, calling scrollIntoView doesn't work properly because the element doesn't actually exist yet. I moved the call to scrollIntoView into a setTimeout, which will scroll the element into view after a short delay. To me, the delay is not noticeable, but I think I can find a better solution, hence I will make this a draft PR for now until I can find a better solution.

Edit:

My hypothesis for the issue is wrong. The real issue is with Suspense mounting unsuspended children while also hiding them with display: none. That means useEffect is still called on them even though the DOM is unstable and accessing the DOM at that point causes unexpected results.

Unfortunately, there is no good way to approach this issue in React 16.14. The issue is supposedly fixed in React 18, so I added a TODO to remove usage of setTimeout when we upgrade to 18.

Related issue: facebook/react#14536

Testing instructions

  • Review code
  • Check Actions build
  • Review changes to test coverage

Agile board tracking

connect to #1115

Closes #1115

@kennethnym kennethnym added the bug Something isn't working label Jul 6, 2022
@kennethnym kennethnym marked this pull request as ready for review July 6, 2022 14:38
@kennethnym kennethnym requested a review from louise-davies July 6, 2022 14:38
@codecov
Copy link

codecov bot commented Jul 6, 2022

Codecov Report

Merging #1118 (1e3261b) into develop (ea067ff) will decrease coverage by 0.91%.
The diff coverage is 84.61%.

@@             Coverage Diff             @@
##           develop    #1118      +/-   ##
===========================================
- Coverage    97.66%   96.74%   -0.92%     
===========================================
  Files           42       43       +1     
  Lines         1586     1599      +13     
  Branches       433      437       +4     
===========================================
- Hits          1549     1547       -2     
- Misses          34       48      +14     
- Partials         3        4       +1     
Impacted Files Coverage Δ
src/App.tsx 80.64% <33.33%> (-8.65%) ⬇️
src/helpPage/helpPage.component.tsx 100.00% <100.00%> (ø)
src/hooks/useAnchor.ts 100.00% <100.00%> (ø)
src/homePage/homePage.component.tsx 62.96% <0.00%> (-14.82%) ⬇️
src/preloader/preloader.component.tsx 90.00% <0.00%> (-10.00%) ⬇️
src/routing/routing.component.tsx 94.93% <0.00%> (-5.07%) ⬇️
src/mainAppBar/mainAppBar.component.tsx 97.40% <0.00%> (-2.60%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ea067ff...1e3261b. Read the comment docs.

Copy link
Member

@louise-davies louise-davies left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When I was trying to refresh the page with one plugin loaded in SciGateway it was about 50/50 whether it jumped down to the correct header or not - so I think there needs to be more thought on fixing this pre-react 18.

We currently only use Suspense for the react-18next library to prevent the app from loading before the translations have loaded. iirc, you can disable the behaviour of using Suspense and go back to getting a loading bool from useTranslation which we can manually use to display the preloader and prevent the help page from mounting. Additionally, there's potential for some page jumping because of the images getting drawn after we've jumped to the anchor - I think this can be prevented by manually specifying the width + height of the images in the img tags

@kennethnym
Copy link
Member Author

Good to know react-i18next has a non-Suspense fallback. That should fix the problem. Specifying the dimensions of the images should fix the image jumping problem as well, especially when we already know the width and height of the images.

@louise-davies louise-davies self-requested a review July 28, 2022 13:10
Copy link
Member

@louise-davies louise-davies left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I refreshed a bunch of times and it worked well every time :) We'll just need to note somewhere that we should renable i18n's suspense when we upgrade to React 18 - either in an issue or on the renovate react 18 PR?

I'll also need to remember to add the sizes to all the images in both the DLS & ISIS help pages 😅

@kennethnym
Copy link
Member Author

Will do. One regression I just noticed when adding fixed sizes to images is that when the viewport gets too narrow it will not resize accordingly but instead becomes horizontally scrollable.

@louise-davies
Copy link
Member

@kennethnym that's probably fine though - we have to trade off the anchors working right and the images getting resized. I think having horizontal scroll because the images are too big is fine? We can make the images a little smaller so they're not full screen on a large monitor, but unless you fancy doing tricks like this: https://stackoverflow.com/a/45869996/7458681, https://www.voorhoede.nl/en/blog/say-no-to-image-reflow/#the-fixed-ratio-fix I think we're stuck with this

@kennethnym
Copy link
Member Author

I think it will be too tricky to do since the text is essentially rendered from an HTML string in a JSON file.

@kennethnym kennethnym merged commit 7ad6be5 into develop Aug 2, 2022
@louise-davies louise-davies deleted the bugfix/jump-to-anchor-#1115 branch August 12, 2022 09:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Anchors on the help page don't scroll correctly on first load
2 participants