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

[select] feat(MultiSelect2): allow creating multiple items #5424

Merged
merged 7 commits into from
Sep 23, 2022

Conversation

ZeRego
Copy link
Contributor

@ZeRego ZeRego commented Jul 10, 2022

Fixes #5384

Checklist

  • Includes tests
  • Update documentation

Changes proposed in this pull request:

  • Change createNewItemFromQuery to support array of new items

Reviewers should focus on:

  • Unsure how wouldCreatedItemMatchSomeExistingItem should behave. Since the logic for splitting the query into multiple items is on the user implementation side, it will display the create item option when there is only a partial match. E.g: query "test, test1" items are "test" and "test1" and test already exists in the list. We would still show the create item option
  • I wrote a test but I don't know why it is failing and how to debug tests. Can you provide more info on how to run tests in debug mode ?

Screenshot

create multiple

@palantirtech
Copy link
Member

Thanks for your interest in palantir/blueprint, @ZeRego! Before we can accept your pull request, you need to sign our contributor license agreement - just visit https://cla.palantir.com/ and follow the instructions. Once you sign, I'll automatically update this pull request.

Copy link
Contributor

@adidahiya adidahiya left a comment

Choose a reason for hiding this comment

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

The best way to run unit tests here would be to focus on one component, let's say Select2 to start, and scope the test runner to just those tests by temporarily editing select2Tests.tsx to say describe.only("<Select2>", ... You can scope down the tests further by editing selectComponentSuite.tsx to say describe.only("create", .., etc. Make sure to revert these edits before pushing to the PR later.

Then you can go into packages/select and run yarn test:karma:debug. This will run the tests once, and start a watcher to re-run them when source or test files change.

packages/select/src/common/listItemsProps.ts Outdated Show resolved Hide resolved
@adidahiya
Copy link
Contributor

ping @ZeRego, any interest in continuing this PR?

@ZeRego ZeRego force-pushed the jr/allow-create-multiple-items branch from aab0882 to 030e9e3 Compare September 21, 2022 20:05
@ZeRego
Copy link
Contributor Author

ZeRego commented Sep 21, 2022

ping @ZeRego, any interest in continuing this PR?

@adidahiya Sorry for the delay.

Testing was a bit of a pain since I couldn't figure out how to run the tests with breakpoints in Webstorm IDE.
I had to add .skip to all the tests beside the select2 and use console logs to figure out what was happening.

Looks like I was missing the event keydown even though the other tests only have the keyup event so I'm now confused on how those tests are passing 🤷

@adidahiya
Copy link
Contributor

@ZeRego I'll take a look at your updates. In the future, please don't force-push to PR branches; I like to see all the commits in their original timeline. I squash all PRs into a single commit when merging.

Copy link
Contributor

@adidahiya adidahiya left a comment

Choose a reason for hiding this comment

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

docs preview looks good!

2022-09-22 11 53 59

the test coverage failure looks like a bug in our test coverage configuration. we shouldn't be checking docs-app code for coverage. please update packages/select/karma.conf.js:

    const baseConfig = createKarmaConfig({
        dirname: __dirname,
        coverageExcludes: [
            "../docs-app/*",
        ],
    });

packages/docs-app/src/common/films.tsx Outdated Show resolved Hide resolved
@adidahiya
Copy link
Contributor

please update packages/select/karma.conf.js:

My suggestion above didn't work, for some reason Karma doesn't allow me to match those globs on files outside the current working directory. I've added a commit here which uses /* istanbul ignore next */ instead.

@adidahiya adidahiya changed the title [MultiSelect2] feat: allow create multiple items [select] feat(MultiSelect2): allow creating multiple items Sep 23, 2022
@adidahiya adidahiya merged commit 6e7deef into palantir:develop Sep 23, 2022
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.

[MultiSelect2] allow to create multiple items
3 participants