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

DOP-4815 #1163

Merged
merged 11 commits into from
Jul 15, 2024
Merged

DOP-4815 #1163

merged 11 commits into from
Jul 15, 2024

Conversation

seungpark
Copy link
Collaborator

@seungpark seungpark commented Jul 11, 2024

Stories/Links:

DOP-4815
DOP-4805

Current Behavior:

Glossary component with anchor tag in link is hidden
DOP-4805 Scrolling to top when linking to table
DOP-4805 original slack thread shows link in note not going to right location for Target component (click link in note to be falsely led. this was the slack thread in DOP-4805)

Staging Links:

Glossary component link
DOP-4805 Removed scrolling to top
DOP-4805 scroll position is updated for Target component (link in note leads you to correct section)

Notes:

  • Adds [the same] buffer of top scroll space needed for glossary items mentioned in DOP-4815
  • Updates the post render effect to update scroll - remove scroll position calculation - use element scrollIntoView with a scroll margin css property

README updates

    • This PR introduces changes that should be reflected in the README, and I have made those updates.
    • This PR does not introduce changes that should be reflected in the README

@seungpark seungpark marked this pull request as ready for review July 11, 2024 19:46
@seungpark
Copy link
Collaborator Author

this does not address the issue highlighted by the original description in DOP-4805 (due to tabs not appearing on first render and pushing content down). since we are reading from local storage to preserve tabbed content space back, may have to put the effect of scrolling back

@seungpark seungpark marked this pull request as draft July 12, 2024 14:14
@seungpark
Copy link
Collaborator Author

this does not address the issue highlighted by the original description in DOP-4805 (due to tabs not appearing on first render and pushing content down). since we are reading from local storage to preserve tabbed content space back, may have to put the effect of scrolling back

After some investigation, found out that the tabs update is possibly causing this issue.
example page https://docs-mongodb-org-stg.s3.us-east-2.amazonaws.com/master/docs/seung.park/DOP-4815/core/databases-and-collections/index.html#click-create.-2

The content is hidden in tabs, and even though the content is available in HTML, its being hidden and thus the browser sometimes can find this and scroll down.

to mitigate this, added the useEffect back, but with updated method of calculation and calculates within the timeout, not before

@seungpark seungpark marked this pull request as ready for review July 12, 2024 16:24
Copy link
Collaborator

@rayangler rayangler left a comment

Choose a reason for hiding this comment

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

Thanks for working on this. I noticed some weird stuttering when attempting to permalink to a heading for the first time. I don't see this happening on prod though. Just wanted to flag this in case this is not something we want to keep.

Screen.Recording.2024-07-12.at.2.25.29.PM.mov

src/components/DefinitionList/DefinitionListItem.js Outdated Show resolved Hide resolved
src/hooks/use-hash-anchor.js Outdated Show resolved Hide resolved
Copy link
Collaborator

@mayaraman19 mayaraman19 left a comment

Choose a reason for hiding this comment

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

Nice! Generally looks good to me. One note is that now the scroll position for the third link after initial click is different, showing more of the previous section than before (not due to pencil banner). Not sure if this is problematic but just wanted to point it out

image
image

@seungpark
Copy link
Collaborator Author

Thanks for working on this. I noticed some weird stuttering when attempting to permalink to a heading for the first time. I don't see this happening on prod though. Just wanted to flag this in case this is not something we want to keep.

Screen.Recording.2024-07-12.at.2.25.29.PM.mov

thanks for catching this! actually went ahead and removed the scroll position calculation, and instead went with elm.scrollIntoView (which should act the same each time) 👍

@seungpark
Copy link
Collaborator Author

Nice! Generally looks good to me. One note is that now the scroll position for the third link after initial click is different, showing more of the previous section than before (not due to pencil banner). Not sure if this is problematic but just wanted to point it out

image image

i believe this is due to the footer being lazy loaded in my staging! (browser attempts to scroll to the note at top of page, except theres no footer)

that said, the point of that link was to follow the link in the note! i updated the description PTAL

Copy link
Collaborator

@mmeigs mmeigs left a comment

Choose a reason for hiding this comment

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

Looks great, thank you for jumping on this

@seungpark seungpark merged commit f00cdbd into main Jul 15, 2024
4 checks passed
@seungpark seungpark deleted the DOP-4815 branch July 15, 2024 14:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants