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

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

Closed
2 tasks done
ariellalgilmore opened this issue Mar 10, 2022 · 7 comments
Closed
2 tasks done

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

ariellalgilmore opened this issue Mar 10, 2022 · 7 comments
Assignees
Labels
bug Something isn't working dev Needs some dev work DPO Consulting owner: Innovation Team used when the engineering work will be done by Hybrid Cloud with DDS engineers as consultants package: utilities Work necessary for the Carbon for IBM.com utilities package package: web components Work necessary for the IBM.com Library web components package severity 3 Affects minor functionality, has a workaround

Comments

@ariellalgilmore
Copy link
Member

ariellalgilmore commented Mar 10, 2022

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

@ariellalgilmore ariellalgilmore added bug Something isn't working package: utilities Work necessary for the Carbon for IBM.com utilities package dev Needs some dev work package: web components Work necessary for the IBM.com Library web components package labels Mar 10, 2022
@proeung proeung added the owner: Innovation Team used when the engineering work will be done by Hybrid Cloud with DDS engineers as consultants label Mar 14, 2022
@proeung proeung added DPO Consulting severity 3 Affects minor functionality, has a workaround labels Mar 14, 2022
@andy-blum
Copy link
Contributor

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
Copy link
Member Author

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
Copy link
Contributor

proeung commented Apr 4, 2022

@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
Copy link
Contributor

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
Copy link
Member Author

@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
Copy link
Contributor

@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
Copy link
Contributor

kodiakhq bot pushed a commit that referenced this issue Apr 18, 2022
…om-shell code (#8684)

### Related Ticket(s)

Closes #8486 #8472

### Description

This PR is a followup top #8434 in which the dotcom-shell's scroll handling was mimicked in a utility class.

This PR aims to cleanup the storybook integration and removes now unnecessary code from the dotcom-shell-composite class.

### Changelog

**Changed**

- Updated storybook story for new dotcom-shell scrolling utility

**Removed**

- Scroll handling logic removed from the dotcom-shell component
@proeung proeung closed this as completed Apr 18, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working dev Needs some dev work DPO Consulting owner: Innovation Team used when the engineering work will be done by Hybrid Cloud with DDS engineers as consultants package: utilities Work necessary for the Carbon for IBM.com utilities package package: web components Work necessary for the IBM.com Library web components package severity 3 Affects minor functionality, has a workaround
Projects
None yet
Development

No branches or pull requests

8 participants