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

fix(CodeSnippet): overflow indicators updated onResize #9605

Merged

Conversation

jnm2377
Copy link
Contributor

@jnm2377 jnm2377 commented Sep 5, 2021

Closes #8441

Codesnippet had a bug that did not update the "overflow indicator" (gradient) when window was resized. This PR addresses that bug.

Removed

  • removed the debounce callback in the resizeObserver because I realized it wasn't actually running the callback after 200ms. I think this is a lodash bug but I couldn't find any helpful issues related to this bug.

How I came to this conclusion

  1. I realized we were updating the overflow indicator (bool) states within handleScroll()
  2. I then looked at where handleScroll was being used
    a. within a useEffect hook
    b. within useResizeObserver which has an onResize callback that updates several states for multi & single line snippets
    c. within onScroll for the single & multiline divs
  3. debugging then involved a lot of console logs to see if the overflow indicator states hasLeftOverflow hasRightOverflow were being updated properly each time handleScroll was being called
    • The overflow states were being updated/consoled properly on initial mount which meant the useEffect instance was working fine
    • Overflow states were also being updated properly when I scrolled on the codesnippet
    • Overflow states were not being updated when I resized the window
  4. Once I realized it was just the window resizing that wasn't updating properly, I added a console.log within the onResize conditional where handleScroll was being called to see if we were even getting within that conditional when resizing.
  if (
    (codeContentRef?.current && type === 'multi') ||
    (codeContainerRef?.current && type === 'single')
  ) {
    console.log('are we even getting here?');
    debounce(handleScroll, 200);
  }

We were "getting there". Which then led me to realize that for some reason, the debounce callback wasn't working.

  1. I removed the debounce callback and directly called handleScroll instead and tested resizing the window. This time the overflow indicator states were updated properly.
  2. Once I confirmed it was the debounce issue, I tried some optional configs for it to see if it would make a difference (maxWait, leading, trailing). But it still wasn't working. So I stuck with just removing it. Since we don't use debounce on the other updates within onResize, I figured it was ok.

Testing / Reviewing

  1. check out PR locally or open deploy preview
  2. in the codesnippet single line story:
    a. check that right overflow indicator displays initially
    b. scroll slightly right and check that left and right indicators display
    c. scroll all the way right, and right indicator should go away
    d. now resize the window so that the codesnippet component becomes smaller. The resizing should update the overflow indicators.

You can test it against this video (recording of bug) and notice the overflow indicator working on window resize.

Screen.Recording.2021-08-25.at.11.27.05.AM.mov

@jnm2377 jnm2377 requested a review from a team as a code owner September 5, 2021 02:55
@netlify
Copy link

netlify bot commented Sep 5, 2021

✔️ Deploy Preview for carbon-react-next ready!

🔨 Explore the source changes: 3970da2

🔍 Inspect the deploy log: https://app.netlify.com/sites/carbon-react-next/deploys/613a7ba182bcc60007b033a9

😎 Browse the preview: https://deploy-preview-9605--carbon-react-next.netlify.app

@jnm2377 jnm2377 changed the title fix: onResize wont debounce handleScroll fix(CodeSnippet): overflow indicators updated onResize Sep 5, 2021
@netlify
Copy link

netlify bot commented Sep 5, 2021

✔️ Deploy Preview for carbon-elements ready!

🔨 Explore the source changes: 3970da2

🔍 Inspect the deploy log: https://app.netlify.com/sites/carbon-elements/deploys/613a7ba15576a800080b09c7

😎 Browse the preview: https://deploy-preview-9605--carbon-elements.netlify.app

@netlify
Copy link

netlify bot commented Sep 5, 2021

✔️ Deploy Preview for carbon-components-react ready!
Built without sensitive environment variables

🔨 Explore the source changes: 3970da2

🔍 Inspect the deploy log: https://app.netlify.com/sites/carbon-components-react/deploys/613a7ba190f6be0007cd414a

😎 Browse the preview: https://deploy-preview-9605--carbon-components-react.netlify.app

Copy link
Contributor

@dakahn dakahn left a comment

Choose a reason for hiding this comment

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

Looks good to me on Edge in Ubuntu 21 👍🏾

@kodiakhq kodiakhq bot merged commit cf503d6 into carbon-design-system:main Sep 9, 2021
This was referenced Sep 15, 2021
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.

React CodeSnippet doesn't update right overflow indicator on resize
3 participants