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

Dynamically truncate text to be the right length for the StyledSelect box #55

Merged
merged 3 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

The StyledSelect component currently truncates text to a fixed length of 24 characters to enable it to fit inside. However, we realised that when the text inside contains wide characters, 24 characters is too long. Additionally, at some viewport widths, 24 characters isn't long enough to fill the StyledSelect component's width and looks weird with the text truncated that short. We needed a way to dynamically truncate text to the correct length depending on the size of the component.

Changes

  • Use canvas to dynamically truncate text in the StyledSelect component
  • Update snapshots
  • Skip failing tests (see "Considerations")

Required Tasks

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

Considerations

Unfortunately, dynamically sizing the text makes the functionality significantly less testable due to the fact that it makes use of canvas. While the node-canvas package enables canvas to be used in Node environments, the package doesn't support worker_threads, which our testing setup apparently requires. The maintainers have indicated they would like to add this support this year, so instead of deleting the tests that no longer work, I've marked them to be skipped so we can come back to them once a new version node-canvas comes out that provides the support we need.

Manual Test Cases

You can test this by following the repro steps present on the Trello card and seeing that the bug doesn't occur:

  1. Create a game called "New Name Has Whitespace Too" (this was the name that originally broke it - it's on account of the Ws, which are the widest letters in non-monospaced fonts)
  2. Visit the shopping lists page
  3. See that the game you just created is selected
  4. See that the game's name is truncated to fit in the header
  5. Expand the dropdown
  6. See that the name is also truncated in the dropdown

You'll want to test this at multiple viewport widths, paying particular attention to viewport widths between 320px and 480px, inclusive, since at these widths the size of the StyledSelect component changes with the viewport.

You'll also want to ensure that selecting games still works, etc.

Screenshots and GIFs

Screenshots up to 769px are shown. Even this is really overkill because the size of the dropdown is static for viewport widths above 481px.

320px

320px

Dropdown-320

480px

Dropdown-480

481px

Dropdown-481

601px

Dropdown-601

769px

Dropdown-769

@@ -1,5 +1,4 @@
import { describe, test, expect } from 'vitest'
import { act, fireEvent } from '@testing-library/react'
Copy link
Owner Author

Choose a reason for hiding this comment

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

These weren't being used.

type MouseEventHandler,
type CSSProperties,
} from 'react'
import { useState, useEffect, type CSSProperties } from 'react'
Copy link
Owner Author

Choose a reason for hiding this comment

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

The removed imports were not being used.

const truncatedText = (text: string, width?: number) => {
if (!width) return

const maxWidth = width - 48
Copy link
Owner Author

@danascheider danascheider Apr 7, 2023

Choose a reason for hiding this comment

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

48: width of button + side margins

@danascheider danascheider force-pushed the 281-fix-whatever-is-going-on-with-styledselect branch from 7413014 to e553138 Compare April 7, 2023 05:05
@danascheider danascheider merged commit c21d529 into main Apr 7, 2023
@danascheider danascheider deleted the 281-fix-whatever-is-going-on-with-styledselect branch April 7, 2023 05:10
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