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

Feature/downshift select #620

Merged
merged 8 commits into from
May 1, 2021
Merged

Feature/downshift select #620

merged 8 commits into from
May 1, 2021

Conversation

westwood846
Copy link
Contributor

@westwood846 westwood846 self-assigned this Mar 5, 2021
@westwood846 westwood846 force-pushed the feature/downshift-select branch 2 times, most recently from b36f3f1 to 0dca21a Compare March 22, 2021 09:00
@westwood846
Copy link
Contributor Author

There's an issue here that moves the cursor to the end of the input whenever the input is changed. This effectively prevents the user from editing anything in the middle of the input. This is a known, unsolved problem with downshift:

@westwood846
Copy link
Contributor Author

I tried making the async example more useful by accessing a public API, but I ran into CORS issues:

interface Issue {
  id: number;
  title: string;
  location: string;
}
const ItemWithLocation: React.FC<ItemProps> = ({ item, ...props }) => (
  <Select.Components.Item {...props} item={item}>
    {item.label} ({item.location})
  </Select.Components.Item>
);
export const AsyncItems: Story<Props> = () => {
  const [jobs, setJobs] = useState<Issue[]>([]);
  const [query, setQuery] = useState<string>('');
  const fetchJobs = async (query: string) => {
    const response = await fetch(
      `https://jobs.github.com/positions.json?description=${query}`
    );
    const jobs = await response.json();
    setJobs(jobs);
  };
  React.useEffect(() => {
    // if (query && query.length < 3) return;
    fetchJobs(query);
  }, [query]);
  const issueItems = jobs.map(issue => ({
    value: issue.id.toString(),
    label: issue.title,
    location: issue.location,
  }));
  return (
    <>
      <Select
        label="Jobs"
        items={issueItems}
        components={{ Item: ItemWithLocation }}
        // helperText="Search by assignee"
        transformFunction={items => items}
        onInputValueChange={setQuery}
        inputValue={query}
      />
    </>
  );
};
AsyncItems.parameters = {
  docs: {
    description: {
      story:
        'The Select is built with async items in mind. Just update the items at any time.',
    },
  },
};

@ritikamotwani
Copy link
Contributor

4 commits are very less for this PR haha. Would have been easier to review by every commit. 🤔

@westwood846
Copy link
Contributor Author

4 commits are very less for this PR haha. Would have been easier to review by every commit.

Yeah I know, but it wasn't really possible for me. There was almost no incremental work here. Either it worked or it didn't. Also this was very exploratory for me.

Just look at the final code.

@rosslo
Copy link
Contributor

rosslo commented Apr 16, 2021

The name of NewSelect is a bit weird to me. Will name it AriesSelect or DownshiftSelect better?

@rosslo
Copy link
Contributor

rosslo commented Apr 16, 2021

Problem: As a user, I cannot delete the input value from the middle.
Current behavior: The cursor will be automatically moved to the end of the input value.

@rosslo
Copy link
Contributor

rosslo commented Apr 16, 2021

I think we can consider adding the following test cases:

  • test disableTyping Prop
  • test disable property of Item

// This is the type for the 'components' prop on the Select. Using
// ComponentType allows us to take in any kind of component, e.g. class
// components, functional components or styled components.
export interface Components {
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure if we should have a separate file for all these types?
Then Select.tsx can only have the Select component.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's only around 30 lines of code for the types. I don't think that would make it easier.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh not because of the number of lines of code. It might be a personal preference I guess. It helps in separating the responsibility of files. The types are kept in a separate file.
Something that can be decided for standardization of the codebase later.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see!

Actually I don't like separating stuff by "type" so much ^^ I think it's better to organize things logically, in modules. Like we're doing on an app level, with the module folders, where we've tried to move away from the whole "reducers", "selectors", "actions" and whatnot-folders.

@westwood846
Copy link
Contributor Author

Problem: As a user, I cannot delete the input value from the middle.
Current behavior: The cursor will be automatically moved to the end of the input value.

See comment further up.

@ritikamotwani
Copy link
Contributor

ritikamotwani commented Apr 17, 2021

@westwood846 the code looks good. Thanks for all the storybook examples, they are like a good documentation to go through all the functionalities.

There are a couple of warnings, can be fixed before merging.

@westwood846 westwood846 merged commit 2e5914c into v4 May 1, 2021
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.

New Select Component
3 participants