Skip to content

Conversation

@sylvhama
Copy link
Contributor

@sylvhama sylvhama commented May 5, 2020

WHY are these changes introduced?

Fixes #1089

Allows user to directly focus the textfield when there aren't options while navigating with a keyboard.

WHAT is this pull request doing?

It makes the tabindex dynamic based on the number of options.

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

import {Page, Card, Autocomplete, TextField, FormLayout, Button} from '../src';

export function Playground() {
  const [options, setOptions] = useState<{value: string; label: string}[]>([]);

  const addOptions = () => {
    setOptions([
      {value: 'montreal', label: 'Montreal'},
      {value: 'tokyo', label: 'Tokyo'},
      {value: 'seoul', label: 'Seoul'},
    ]);
  };

  return (
    <Page title="Playground">
      <Card sectioned>
        <FormLayout>
          <TextField label="Name" />
          <Autocomplete
            options={options}
            selected={[]}
            onSelect={() => null}
            listTitle="Suggested Cities"
            textField={<Autocomplete.TextField label="City" />}
            willLoadMoreResults
          />
          <TextField label="Phone" />
          {options.length === 0 && (
            <Button onClick={addOptions}>Add options</Button>
          )}
        </FormLayout>
      </Card>
    </Page>
  );
}

🎩 checklist

  • Tested on mobile
  • Tested on multiple browsers
  • Tested for accessibility
  • Updated the component's README.md with documentation changes
  • Tophatted documentation changes in the style guide
  • For visual design changes, pinged one of @ HYPD, @ mirualves, @ sarahill, or @ ry5n to update the Polaris UI kit

@ghost
Copy link

ghost commented May 5, 2020

👋 Thanks for opening your first pull request. A contributor should give feedback soon. If you haven’t already, please check out the contributing guidelines.

@github-actions
Copy link
Contributor

github-actions bot commented May 5, 2020

🟢 This pull request modifies 3 files and might impact 1 other files.

Details:
All files potentially affected (total: 1)
📄 UNRELEASED.md (total: 0)

Files potentially affected (total: 0)

🧩 src/components/Autocomplete/components/ComboBox/ComboBox.tsx (total: 1)

Files potentially affected (total: 1)

🧩 src/components/Autocomplete/components/ComboBox/tests/ComboBox.test.tsx (total: 0)

Files potentially affected (total: 0)

@sylvhama sylvhama force-pushed the sylvhama/combobox/empty-options-tabindex branch from a0dfb46 to 7da3f08 Compare May 5, 2020 16:06
@sylvhama
Copy link
Contributor Author

sylvhama commented May 5, 2020

Reminder: there was already an attempt to fix this via #1126
My solution does not remove the tabindex, it will allow the browser to "ignore" that div where there aren't optons.

@danrosenthal
Copy link

Hey @sylvhama, thanks for the contribution. I don't have the bandwidth to review this currently, so am removing myself as a reviewer. My suggestion would be to ping a couple people on your team for review and a thorough tophat, with particular focus on testing for accessibility. Once two members of your team have reviewed and approved, we can circle back and talk about getting this released.

@danrosenthal danrosenthal removed their request for review May 6, 2020 13:23
@sylvhama sylvhama requested a review from krystalcampioni May 6, 2020 15:21
@dpersing
Copy link

dpersing commented May 6, 2020

Hi @sylvhama! I'm afraid I have to second Dan's comment. I'd recommend careful testing within your team first. If you have accessibility questions that can't be answered by what's in the Accessibility Handbook, please reach out to the #accessibility channel in Slack.

@dpersing dpersing removed their request for review May 6, 2020 17:37
Copy link

@krystalcampioni krystalcampioni left a comment

Choose a reason for hiding this comment

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

🎩 works as expect

@sylvhama sylvhama requested a review from carolopolo May 7, 2020 15:08
Copy link
Contributor

@carolopolo carolopolo left a comment

Choose a reason for hiding this comment

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

LGTM! 🤠

@sylvhama sylvhama force-pushed the sylvhama/combobox/empty-options-tabindex branch from b04f4ff to 81caa24 Compare May 8, 2020 14:25
@sylvhama sylvhama merged commit 19f899e into master May 8, 2020
@sylvhama sylvhama deleted the sylvhama/combobox/empty-options-tabindex branch May 8, 2020 14:53
@ghost
Copy link

ghost commented May 8, 2020

🎉 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

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Autocomplete][v1] ComboBox uses tabindex=0 which causes tabbing issues with AutoComplete

5 participants