carbon icon indicating copy to clipboard operation
carbon copied to clipboard

fix(select): ensure select list always has correct height to fit content

Open robinzigmond opened this issue 3 years ago • 2 comments

Use a callback ref to ensure that the the code that calculates the list height is ran whenever the DOM node is attached.

fix #5341

Proposed behaviour

Select list should always be the correct height to show all its options, with the correct padding/margin, no matter how many times the text wraps.

Current behaviour

In some circumstances (needs few enough options for there not to be a scrollbar), options with long text that wraps cause the list height to be too small and some parts of some options therefore are not visible.

Checklist

  • [X] Commits follow our style guide
  • [X] Related issues linked in commit messages if required
  • [X] Screenshots are included in the PR if useful
  • [X] All themes are supported if required
  • [X] Unit tests added or updated if required
  • [X] Cypress automation tests added or updated if required
  • [X] Storybook added or updated if required
  • [X] Translations added or updated (including creating or amending translation keys table in storybook) if required
  • [X] Typescript d.ts file added or updated if required

QA

  • [ ] Tested in CodeSandbox/storybook
  • [ ] Add new Cypress test coverage if required
  • [ ] Carbon implementation matches Design System/designs
  • [ ] UI Tests GitHub check reviewed if required

Additional context

There's a bit of history behind this bug/change that reviewers may find useful.

This was first reported as issue #4489. Fixed in this PR by adding listRef.current to the useLayoutEffect's dependency array - this works because the height needs to be recalculated when the select list container appears in the DOM, which wasn't happening previously. This fix was removed in this PR, as part of a much wider set of changes, it seems without any comment at all!

While restoring the dependency fixes the problem, I assume the reason for the removal was that it causes lint warnings saying that a ref.current shouldn't be used as an effect dependency - effects only run after a render but an update to a ref does not cause a rerender to happen, so this only works when we are fortunate enough for a prop or state change to happen at about the same time, and I didn't want to rely on this.

It seems the way to guarantee code to run when a ref is updated is to use a callback ref, so that is what I have implemented here.

Testing instructions

The following CodeSandbox is an example of the broken behaviour. You can see the new behaviour by looking at the version in the comment by codesandbox[bot].

Codesandbox from original issue: https://codesandbox.io/s/select-options-height-resize-uy2shp?file=/src/index.js

I have changed the "Select/Test" story to a configuration that is more likely to trigger the bug, so this can also be tested (and I have added a new Cypress test to hopefully catch any regressions in this behaviour).

robinzigmond avatar Aug 10 '22 13:08 robinzigmond

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 61e33b6c61e7c84b29950a5829db30ceb6217dea:

Sandbox Source
carbon-quickstart Configuration
carbon-quickstart-typescript Configuration
carbon-quickstart PR
select-options-height-resize PR
select-options-height-resize Issue #5341

codesandbox-ci[bot] avatar Aug 10 '22 13:08 codesandbox-ci[bot]



Test summary

2912 0 2 0Flakiness 0


Run details

Project carbon
Status Passed
Commit 61e33b6c61
Started Sep 1, 2022 2:31 PM
Ended Sep 1, 2022 2:37 PM
Duration 06:01 💡
OS Linux Debian - 10.10
Browser Chrome 100

View run in Cypress Dashboard ➡️


This comment has been generated by cypress-bot as a result of this project's GitHub integration settings. You can manage this integration in this project's settings in the Cypress Dashboard

cypress[bot] avatar Aug 10 '22 13:08 cypress[bot]

:tada: This PR is included in version 110.2.1 :tada:

The release is available on:

Your semantic-release bot :package::rocket:

carbonci avatar Sep 01 '22 15:09 carbonci