Skip to content

Conversation

@patrickracicot
Copy link
Contributor

@patrickracicot patrickracicot commented Aug 19, 2021

WHY are these changes introduced?

Fixes part of #22937
Prevents horizontal scrollbar for very long option text inside an OptionList.

WHAT is this pull request doing?

This PR ensures that the label in the Option of the OptionList breaks when it's too long for the space of the option list.

Before
image

After
image

How to 🎩

🖥 Local development instructions
🗒 General tophatting guidelines
📄 Changelog guidelines

Copy-paste this code in playground/Playground.tsx:
import {CirclePlusMinor} from '@shopify/polaris-icons';
import React, {useState} from 'react';

import {Autocomplete, Icon, Key, KeypressListener, Page} from '../src';

export function Playground() {
  const [textFieldValue, setTextFieldValue] = useState('');
  const [textFieldFocused, setTextFieldFocused] = useState(false);
  const [currentTags, setCurrentTags] = useState<string[]>([]);
  const options = [
    {value: 'cheese_pizza', label: 'Cheese Pizza'},
    {value: 'macaroni_pizza', label: 'Macaroni Pizza'},
    {value: 'pepperoni_pizza', label: 'Pepperoni Pizza'},
    {
      value: 'ok',
      active: true,
      media: <Icon source={CirclePlusMinor} color="base" />,
      label:
        'asdasdasddasdasdasddasdasdasddasdasdasddasdasdasddasdasdasddasdasdasddasdasdasdd',
    },
  ];

  function handleTextFieldEnter() {
    setTextFieldValue('');
  }
  function handleTextFieldChange(nextTextFieldValue: string) {
    setTextFieldValue(nextTextFieldValue);
  }
  function handleTextFieldFocused() {
    setTextFieldValue('');
    setTextFieldFocused(true);
  }

  const textFieldMarkup = (
    <>
      <KeypressListener keyCode={Key.Enter} handler={handleTextFieldEnter} />
      <Autocomplete.TextField
        autoComplete="off"
        label="Text Field label"
        labelHidden
        onBlur={() => {
          setTextFieldFocused(false);
        }}
        onChange={handleTextFieldChange}
        onFocus={handleTextFieldFocused}
        placeholder="The placeholder"
        value={textFieldFocused ? textFieldValue : ''}
      />
    </>
  );

  return (
    <Page title="Playground">
      <Autocomplete
        listTitle="Title"
        onSelect={(selected: string[]) => {
          setCurrentTags(selected);
          setTextFieldValue('');
        }}
        options={options}
        preferredPosition="mostSpace"
        selected={currentTags}
        textField={textFieldMarkup}
      />
    </Page>
  );
}

🎩 checklist

@patrickracicot patrickracicot self-assigned this Aug 19, 2021
@patrickracicot patrickracicot added the Bug Something is broken and not working as intended in the system. label Aug 19, 2021
@github-actions
Copy link
Contributor

github-actions bot commented Aug 19, 2021

size-limit report

Path Size
cjs 142.64 KB (0%)
esm 96.38 KB (0%)
esnext 139.59 KB (+0.01% 🔺)
css 33.77 KB (+0.02% 🔺)

cursor: pointer;
border-radius: var(--p-border-radius-base);
padding: spacing(tight);
word-break: break-word;
Copy link
Member

Choose a reason for hiding this comment

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

👋 Just a heads up this property is deprecated

Better to use overflow-wrap: anywhere which should work in the same way

Copy link
Contributor Author

@patrickracicot patrickracicot Aug 19, 2021

Choose a reason for hiding this comment

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

🤔 anywhere isn't supported by Safari. In that case should we include both for backward compatibility ?
https://developer.mozilla.org/en-US/docs/Web/CSS/overflow-wrap#browser_compatibility

Unless we go with word-break: break-all;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As discussed on slack, overflow-wrap: anywhere is not an option as it isn't supported by Safari. Instead, I went with word-break: break-all which achieves intended fix and also works with Chinese/Japanese/Korean (CJK) text.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So after another find 💡 by Kyle, I've instead used the mixin text-breakword

@patrickracicot patrickracicot force-pushed the pracicot/autocomplet_wordbreak branch from 954c16d to ed300df Compare August 19, 2021 17:29
@patrickracicot patrickracicot marked this pull request as ready for review August 19, 2021 17:57
@patrickracicot patrickracicot force-pushed the pracicot/autocomplet_wordbreak branch 2 times, most recently from 7f43275 to 5477faf Compare August 19, 2021 18:17
@patrickracicot patrickracicot force-pushed the pracicot/autocomplet_wordbreak branch from 5477faf to 6882523 Compare August 19, 2021 20:24
Copy link
Member

@chloerice chloerice left a comment

Choose a reason for hiding this comment

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

Thank you @patrickracicot! 🚀

@patrickracicot patrickracicot merged commit f601190 into main Aug 24, 2021
@patrickracicot patrickracicot deleted the pracicot/autocomplet_wordbreak branch August 24, 2021 00:15
@ghost
Copy link

ghost commented Aug 24, 2021

🎉 Thanks for your contribution to Polaris React!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Bug Something is broken and not working as intended in the system.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants