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 option name truncation on StyledSelectOption component #56

Merged
merged 6 commits into from
Apr 7, 2023

Conversation

danascheider
Copy link
Owner

@danascheider danascheider commented Apr 7, 2023

Context

Fix whatever is going on with StyledSelect

In #55, I set up the StyledSelect component to dynamically truncate its header text, making sure the text is long enough for the containing element while preventing it from overflowing or wrapping. However, what I overlooked is that the StyledSelectOption component also truncates its optionName prop for display. This resulted in continued wrapping in that component after #55 was merged.

Changes

  • Extract measureText function since the same code is now used in 3 places
  • Use useSize hook in StyledSelectOption component to measure the width of the element and truncate text accordingly
  • Skip tests that failed (see "Considerations")
  • Update snapshots

Required Tasks

  • Add/update automated tests
  • Add/update Storybook stories
  • Add/update docs
  • Verify TypeScript compiles

Considerations

As mentioned in #55, use of the canvas element to measure the width of the text comes at a cost: the node-canvas package that enables this element to be used server-side - i.e., in headless automated tests - does not support worker_threads, which we require due to dependencies of our toolset. The node-canvas maintainers have indicated they will try to add this support soon, so instead of removing affected tests, I have set them to be skipped, as well as updating the titles of the tests to reflect the changes. Hopefully a new node-canvas will be released soon that offers this support.

Manual Test Cases

Dropdown

The width of the dropdown changes when the viewport width is between 320 and 480 pixels. For viewports 481px and wider, the width of the dropdown is fixed. For this reason, you'll need to view the changes at numerous viewport widths in the 320px-480px range, but testing at viewport widths larger than 481px isn't really necessary - if it works at 481px, it works at all the widths higher than that.

  1. Create a game called "New Title Has Whitespace Too" (the value that originally tipped us off to this bug)
  2. Go to the shopping lists page
  3. See that the new game is selected and there is no overflow or wrapping in the visible part of the StyledSelect component
  4. Expand the dropdown
  5. See that option names are appropriately truncated - not too short for the available space, but also no overflow or wrapping

List Edit Form

Because changes have been made to the ListEditForm component, you will need to try editing a shopping list title to make sure that the changes haven't broken the component. No changes to appearance or functionality are expected.

Screenshots and GIFs

Note that, although changes were made to the ListEditForm component, those changes don't affect the appearance or functionality of that component and as such don't need to be tested here.

Mobile Viewports

320px

Dropdown-320

480px

Dropdown-480

Larger Viewports

As mentioned above, the width of the dropdown is fixed at viewport widths 481px and higher, so there's no need to provide more than one of these really.

481px

Dropdown-481

601px

Dropdown-601


@media (min-width: 769px) {
.root {
font-size: 1.025rem;
Copy link
Owner Author

Choose a reason for hiding this comment

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

This was just kind of a fly in the ointment - minimal visible difference but messed with the truncation.

import classNames, { type Argument } from 'classnames'
import { measureText } from '../../utils/measureText'
Copy link
Owner Author

Choose a reason for hiding this comment

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

If I was going to do this over I'd change it to a default export but it doesn't matter enough to make the change in all three places it's imported lol.

@danascheider danascheider force-pushed the 281-fix-whatever-is-going-on-with-styled-select branch from d675638 to ccf01f2 Compare April 7, 2023 09:45
@danascheider danascheider merged commit a7e1c06 into main Apr 7, 2023
@danascheider danascheider deleted the 281-fix-whatever-is-going-on-with-styled-select branch April 7, 2023 09:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant