TechDocs navigation bugfix #20287
TechDocs navigation bugfix #20287
Conversation
…r to a useEffect . Signed-off-by: Sydney Achinger <sydneynicoleachinger@spotify.com>
Unexpected ChangesetsThe following changeset(s) reference packages that have not been changed in this PR:
Note that only changes that affect the published package require changesets, for example changes to tests and storybook stories do not require changesets. Changed Packages
|
|
Uffizzi Preview |
|
This PR has been automatically marked as stale because it has not had recent activity from the author. It will be closed if no further activity occurs. If the PR was closed and you want it re-opened, let us know and we'll re-open the PR so that you can continue the contribution! |
|
Not stale I was just on vacation :) Will pick up soon! |
Signed-off-by: Sydney Achinger <sydneynicoleachinger@spotify.com>
Signed-off-by: Sydney Achinger <sydneynicoleachinger@spotify.com>
| @@ -167,7 +166,6 @@ export const useTechDocsReaderDom = ( | |||
| const postRender = useCallback( | |||
| async (transformedElement: Element) => | |||
| transformer(transformedElement, [ | |||
| scrollIntoAnchor(), | |||
There was a problem hiding this comment.
The Transformers only run after adding TechDocs shadow root to the dom. This means that expected navigation behavior (backwards and forwards browser arrows) will not trigger this logic to run.
| if (window.location.hash) { | ||
| const hash = window.location.hash.slice(1); | ||
| dom?.querySelector(`[id="${hash}"]`)?.scrollIntoView(); | ||
| } |
There was a problem hiding this comment.
This logic does not get called when navigating with browser back and forth arrows. Issue documented here: #20159. In order to fix this, the logic needs to run whenever the window.location.hash changes. I moved the logic to a useEffect.
| if (hashElement && !isStyleLoading) { | ||
| hashElement.scrollIntoView(); | ||
| } else { | ||
| document?.querySelector('header')?.scrollIntoView(); |
There was a problem hiding this comment.
Scrolling the Backstage header element into view effectively scrolls user to the top of the page. If the url does not contain a hash, then scroll to the top of page by default.
There was a problem hiding this comment.
Is this because it always takes the first one?
There was a problem hiding this comment.
Yes it always takes the first one. I was hesitant to do a more specific selection because the header component is different internally vs open source Backstage.
Signed-off-by: Sydney Achinger <sydneynicoleachinger@spotify.com>
| @@ -14,3 +14,5 @@ | |||
| * limitations under the License. | |||
| */ | |||
| import '@testing-library/jest-dom'; | |||
|
|
|||
| Element.prototype.scrollIntoView = jest.fn(); | |||
There was a problem hiding this comment.
This mock is needed because scrollIntoView is not implemented for jsdom. Jsdom is used by tests. More info on issue: jsdom/jsdom#1695. This seems to be the recommended approach as seen here.
Signed-off-by: Sydney Achinger <sydneynicoleachinger@spotify.com>
…techdocs/navigation-bugfix
Signed-off-by: Sydney Achinger <sydneynicoleachinger@spotify.com>
Signed-off-by: Sydney Achinger <sydneynicoleachinger@spotify.com>
|
Thank you for contributing to Backstage! The changes in this pull request will be part of the |
Fix TechDocs page scroll. Move logic from scrollIntoAnchor transformer to a useEffect.
Fixes #19686
Now, when navigating to a new TechDocs page, you are taken to the top of the page.
scroll-top-new-page.mov
Fixes #20159
Scrolling behavior is fixed when navigating using the forward and back browser buttons.
backward-forward-browser-nav.mov
Hey, I just made a Pull Request!
✔️ Checklist
Signed-off-byline in the message. (more info)