search

carbon-design-system / carbon-for-ibm-dotcom

Carbon for IBM.com is based on the Carbon Design System for IBM
https://www.ibm.com/standards/carbon/
Apache License 2.0
268 stars 156 forks source link

[Dotcom-shell]: remove scrolling logic and use new utility #8472

Closed ariellalgilmore closed 2 years ago

ariellalgilmore commented 2 years ago

Engineering info:

Description

Follup up work on #8240.

8240 created a new scrolling utility, but did not yet remove scrolling logic from dds-dotcom-shell. We need to remove the logic from dds-dotcom-shell as well as test functionality with LeadspaceWithSearch in the new story Dotcom-shell -> withoutShell

Component(s) impacted

Dotcom-shell masthead (LO, L1) TOC (vertical, horizontal) Leadspace with search Universal Banner

Browser

No response

Carbon for IBM.com version

Canary

Severity

Severity 3 = The problem is visible or noticeable to users but does not impede the usability or functionality. Affects minor functionality, has a workaround.

Application/website

carbon for ibm.com

Package

@carbon/ibmdotcom-web-components

CodeSandbox example

N/A

Steps to reproduce the issue (if applicable)

No response

Release date (if applicable)

No response

Code of Conduct

andy-blum commented 2 years ago

One thing we need to consider prior to yanking out the dotcom shell's scrolling logic is how do we want to handle the leadspace-with-search component? That's not currently accounted for in the new scrolling utility. How should it interact with the masthead/toc to stick to the top of the page?

@oliviaflory @kennylam

ariellalgilmore commented 2 years ago

Hi @andy-blum! sorry didn't see your comment before. There are specs on the interaction in here: https://ibm.ent.box.com/file/814160426264?s=r2wmgutqhybbmey96zv8rvjjufsw6ib4.

@oliviaflory lmk if there is anything else!

proeung commented 2 years ago

@andy-blum Can you note the issue you're having with getting the scrolling utility and making it work with the Leadspace-with search component?

andy-blum commented 2 years ago

Right now, we're trying to handle everything on scroll, through a throttled event listener. The throttler worked enough before we tried to add in the leadspace-with-search component, but as we add in more logical complexity to the handler, we're blocking the main thread of the JS and causing some jitter in the page. Every single time the scroll handler runs, it's got to go through the entire calculation of determining which components are on the page, if they should stick, and then altering their top value so we can make sure they all behave as a single unit. That's a lot of work that we could (and should) offload to properties. Ideally it would be best to store properties like hasL0Masthead, hasL1Masthead, hasHorizontalToc, as well as TocShouldStick, L0ShouldHide, etc, and then only do the top property calculations.

With regards to the leadspace-with-search component in particular, the main issue I encountered was that the search bar does not cleanly separate from the document flow. When it moves to a fixed position, I was unable to get it to stick to the header, adhere to the carbon grid, and not cause the document to reflow when I set it to position:fixed. I started trying to refactor that component somewhat, but it felt like every improvement I made was a 1-step-forward-2-steps-back kind of thing.

ariellalgilmore commented 2 years ago

@andy-blum Thanks for sharing. Do you have a branch where we could try to see the problem locally to get a better understanding of the issue?

andy-blum commented 2 years ago

@ariellalgilmore I had deleted my local branch, but remembered the majority of the work I did to get started. I've made a new local branch and it's about as close as I've gotten so far. I'll publish it to my fork and share the link with you here.

andy-blum commented 2 years ago

https://github.com/andy-blum/carbon-for-ibm-dotcom/tree/scroll-util-followup