Skip to content

Conversation

KristinLBradley
Copy link
Contributor

@KristinLBradley KristinLBradley commented Oct 8, 2024

📌 Summary

If merged, this PR will prevent unwanted scrollbars from being triggered by hidden panels.
(Merges into AppSideNav component branch.)

AppSideNav within framed layout, notice you can still scroll all content as before.

🛠️ Detailed description

Tested locally within Terraform/Atlas.

📸 Screenshots

Fix within Terraform/Atlas
(Lime green outline added around visible panel. Purple dashed outlines show layout of hidden panels. Their grid area heights are determined by the height of the visible panel.)

image

NOTE: I tested mainly using the SideNav within Terraform. (Will continue testing with AppSideNav to confirm but should be the same.)

🔗 External links


👀 Component checklist

  • Percy was checked for any visual regression
  • [ ] A changelog entry was added via Changesets if needed (see templates here)

💬 Please consider using conventional comments when reviewing this PR.

Copy link

vercel bot commented Oct 8, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated (UTC)
hds-showcase ✅ Ready (Inspect) Visit Preview Oct 8, 2024 10:53pm
hds-website ✅ Ready (Inspect) Visit Preview Oct 8, 2024 10:53pm

@KristinLBradley KristinLBradley changed the title AppSideNav - Fix issue with scrolling caused by hidden panels (HDS-3892) AppSideNav - Fix bug with scrolling caused by hidden panels (HDS-3892) Oct 8, 2024
@KristinLBradley
Copy link
Contributor Author

Not sure how/if I could create a useful test case for the Showcase for the fix.

@didoo
Copy link
Contributor

didoo commented Oct 9, 2024

Not sure how/if I could create a useful test case for the Showcase for the fix.

@KristinLBradley I don't know if in the showcase you can emulate the behaviour of route changes in the sidenav like in a production environment. I think I tried, and I came to the conclusion is not possible with the current setup.

One thing I would test is what happens when the panels have very different heights, and they are scrolled, in the different scenarios. The reason I am saying is that by setting max-height: 0 all of a sudden, in some cases/scenarios it may make the sidenav content jump: I remember I saw this effect when testing the possible solutions for this problem, a few days ago, hence my suggestions about having transitions for this. But I don't know if your solution has this side-effect, so I'll leave it to you to to test this condition (you have to do it in a product environment, unfortunately, unless you come up with a way to emulate this behaviour without a route change.

Below a schema (let me know if it's not clear what I mean 😀) of these possible conditions (there may be more):
screenshot_4279

Take everything with a pinch of salt, I may be completely wrong, so please test before making changes if there is such effect, and if it's worth to mitigate.

@KristinLBradley
Copy link
Contributor Author

KristinLBradley commented Oct 9, 2024

Not sure how/if I could create a useful test case for the Showcase for the fix.

@KristinLBradley I don't know if in the showcase you can emulate the behaviour of route changes in the sidenav like in a production environment. I think I tried, and I came to the conclusion is not possible with the current setup.

One thing I would test is what happens when the panels have very different heights, and they are scrolled, in the different scenarios. The reason I am saying is that by setting max-height: 0 all of a sudden, in some cases/scenarios it may make the sidenav content jump: I remember I saw this effect when testing the possible solutions for this problem, a few days ago, hence my suggestions about having transitions for this. But I don't know if your solution has this side-effect, so I'll leave it to you to to test this condition (you have to do it in a product environment, unfortunately, unless you come up with a way to emulate this behaviour without a route change.

Take everything with a pinch of salt, I may be completely wrong, so please test before making changes if there is such effect, and if it's worth to mitigate.

@didoo In testing, if you've scrolled within the SideNav and then click on a link that navigates to a new panel and then go back to the previous panel then it does not maintain the position you scrolled to so you may have to scroll again to find it. However without the fix, the issues seem worse. In some cases, all the SideNav links were hidden when I navigated making it look empty and I had to scroll to see the links.

@didoo
Copy link
Contributor

didoo commented Oct 10, 2024

In testing, if you've scrolled within the SidNav and then click on a link that navigates to a new panel and then go back to the previous panel then it does not maintain the position you scrolled to so you may have to scroll again to find it. However without the fix, the issues seem worse. In some cases, all the SideNav links were hidden when I navigated making it look empty and I had to scroll to see the links.

Could we leverage something like scrollIntoView or scrollTo?

@KristinLBradley
Copy link
Contributor Author

KristinLBradley commented Oct 10, 2024

In testing, if you've scrolled within the SidNav and then click on a link that navigates to a new panel and then go back to the previous panel then it does not maintain the position you scrolled to so you may have to scroll again to find it. However without the fix, the issues seem worse. In some cases, all the SideNav links were hidden when I navigated making it look empty and I had to scroll to see the links.

Could we leverage something like scrollIntoView or scrollTo?

Perhaps. It's very difficult to figure out how to test everything. I'm poking around within Atlas Terraform more though to see if I can figure anything out.

With what I've added so far in this PR though I still think it's an improvement at least.

Also as far as I can tell this is also an issue currently BTW... @didoo


Update: I've been experimenting but haven't come up with a good way to maintain the previously scrolled to position so far. In my opinion what I have so far at least solves one of the current issues so unless anyone sees drawbacks to it I would like to merge.

Copy link
Member

@alex-ju alex-ju 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 investigating this, Kristin! Would you be able to test the change in HCP and Vault as well?

@KristinLBradley
Copy link
Contributor Author

Thanks for investigating this, Kristin! Would you be able to test the change in HCP and Vault as well?

Tested in HCP, also tested current behavior in Vault which has the same issue.

Without the fix, if you navigate from a long list of links in the side panel to a short list of links, the side panel can appear empty until you scroll back to the top of the content (which may not be obvious that you need to do.) Once you do scroll back and then click to return to the previous screen the list of links displays as before and does not preserve the position you scrolled to within the long list of links.

With the fix, if you navigate from a long list of links to a short one, the short list of links displays as expected (i.e., you don't have to scroll to see the links). If you navigate back to the previous list of links, any position you had scrolled to previously is not preserved and the links display with the scroll position at the top which is the same as the current behavior.

So from the above, I don't see any drawback from the current fix. I think it's a worthwhile improvement as it fixes a serious usability problem which has the potential to confuse and annoy users.

@KristinLBradley KristinLBradley merged commit b7637e7 into hds-3800-app-sidenav-component Oct 15, 2024
14 checks passed
@KristinLBradley KristinLBradley deleted the hds-3892-app-sidenav-scrolling-bug branch October 15, 2024 16:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants