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

feat(Select): add creatable and new features #2820

Merged
merged 4 commits into from Sep 6, 2019

Conversation

@kmcfaul
Copy link
Contributor

kmcfaul commented Sep 3, 2019

What: Adds the creatable and new use cases to select via isCreatable and onCreateOption properties. Specifically applies to typeahead variants.

Refer to issue: #2665

@patternfly-build

This comment has been minimized.

Copy link
Contributor

patternfly-build commented Sep 3, 2019

PatternFly-React preview: https://patternfly-react-pr-2820.surge.sh

@kmcfaul kmcfaul force-pushed the kmcfaul:select-creatable branch from 025fa19 to a8a0ab4 Sep 3, 2019
@kmcfaul kmcfaul requested a review from dlabaj Sep 4, 2019
@tlabaj tlabaj requested a review from mcarrano Sep 4, 2019
Copy link
Member

mcarrano left a comment

This is looking good, But a couple of questions:

  • When I add a new item to the list, looks like it's always added at the end. Is this always the case? If we have an alphabetically list, shouldn't the new item be inserted where it belongs? So if I add Connecticut, why does it get added to the end rather than after Alabama?

  • Shouldn't these options also apply to the multi-value type ahead select?

@kmcfaul

This comment has been minimized.

Copy link
Contributor Author

kmcfaul commented Sep 4, 2019

@mcarrano
When a new item is entered, the addition occurs on the user-side. In the example, I just appended the option to the list. Select does not sort options.

Both new and creatable should work for multi-type ahead select as well. I will add the checkbox toggles to that example!

@mcarrano mcarrano self-requested a review Sep 4, 2019
Copy link
Member

mcarrano left a comment

Looks good. Thanks @kmcfaul

@mcarrano mcarrano added ux approved and removed ux review labels Sep 4, 2019
Copy link
Contributor

dlabaj left a comment

A few comments. Otherwise looks good. Thanks.

@kmcfaul kmcfaul force-pushed the kmcfaul:select-creatable branch from d728c12 to 109521f Sep 5, 2019
@jschuler

This comment has been minimized.

Copy link
Collaborator

jschuler commented Sep 5, 2019

I'm getting the browser autofill popup, should be turned off?
Screen Shot 2019-09-05 at 4 04 14 PM

@christiemolloy

This comment has been minimized.

Copy link
Member

christiemolloy commented Sep 5, 2019

How is isCreatable different from onCreateOption?

When I have onCreateOption selected I can't create an option:
Screen Shot 2019-09-05 at 4 05 03 PM

@christiemolloy

This comment has been minimized.

Copy link
Member

christiemolloy commented Sep 5, 2019

For the multiple typeahead select input when I have Alabama filtered and then I search for Alabama again and add it, both of them delete and it wipes Alabama clear? I don't know if this is the expected behavior?

Screen Shot 2019-09-05 at 4 06 57 PM

@christiemolloy

This comment has been minimized.

Copy link
Member

christiemolloy commented Sep 5, 2019

When the menu expands up, should the arrow rotate @mcarrano

Screen Shot 2019-09-05 at 4 09 32 PM

@kmcfaul

This comment has been minimized.

Copy link
Contributor Author

kmcfaul commented Sep 5, 2019

@jschuler autocomplete is set to off for all selects, so I'm unsure why it would be showing up. Which browser are you using?

@christiemolloy isCreatable handles whether the user can add inputs, but will only do so temporarily without onCreateOption defined. onCreateOption is a callback for the user to store the new option in the overall options list, and is used together with isCreateable.

Re: multi-typeahead, it looks like you are selecting Alabama twice, first which selects it and second which unselects it and removes it form the chip group.

@jschuler

This comment has been minimized.

Copy link
Collaborator

jschuler commented Sep 5, 2019

@kmcfaul strange, Chrome 76.0.3809.132. If no one else sees this then it might be something on my end

@kmcfaul

This comment has been minimized.

Copy link
Contributor Author

kmcfaul commented Sep 5, 2019

That's my version of Chrome and I'm not seeing the autocomplete on the surge. Very odd. Anyone else seeing it?

@christiemolloy

This comment has been minimized.

Copy link
Member

christiemolloy commented Sep 5, 2019

I see autocomplete in Chrome @kmcfaul @jschuler

@jschuler

This comment has been minimized.

Copy link
Collaborator

jschuler commented Sep 5, 2019

@kmcfaul do you have autofill enabled? chrome://settings/addresses

@kmcfaul

This comment has been minimized.

Copy link
Contributor Author

kmcfaul commented Sep 5, 2019

I do have it enabled.

@jschuler

This comment has been minimized.

Copy link
Collaborator

jschuler commented Sep 5, 2019

Might have to wrap in form tags as this comment suggests https://gist.github.com/niksumeiko/360164708c3b326bd1c8#gistcomment-3004386

@kmcfaul

This comment has been minimized.

Copy link
Contributor Author

kmcfaul commented Sep 5, 2019

That seems less than ideal, html structure wise. Will try it out though, hopefully shouldn't change anything visually.

@jschuler

This comment has been minimized.

Copy link
Collaborator

jschuler commented Sep 5, 2019

@kmcfaul ok, i don't recall seeing autocomplete before, i wonder why it's showing now

@kmcfaul kmcfaul force-pushed the kmcfaul:select-creatable branch from 109521f to bbdaad9 Sep 5, 2019
@mcarrano

This comment has been minimized.

Copy link
Member

mcarrano commented Sep 5, 2019

Regarding your question @christiemolloy:

When the menu expands up, should the arrow rotate @mcarrano

I don't feel strongly about this, but I think we should just be consistent with what we do for other dropdown or select instances.

@kmcfaul

This comment has been minimized.

Copy link
Contributor Author

kmcfaul commented Sep 5, 2019

I think it makes sense to have the arrow icon change to an upward direction on direction toggle. This is more of a quick bugfix than related to this particular PR though. I can put up a different PR for it.

@jschuler

This comment has been minimized.

Copy link
Collaborator

jschuler commented Sep 5, 2019

@kmcfaul wrapping inputs with form elements fixed the auto complete issue for me

@tlabaj
tlabaj approved these changes Sep 6, 2019
Copy link
Contributor

tlabaj left a comment

LGTM

@dlabaj
dlabaj approved these changes Sep 6, 2019
Copy link
Member

christiemolloy left a comment

LGTM

@dlabaj dlabaj merged commit f16e0f3 into patternfly:master Sep 6, 2019
8 checks passed
8 checks passed
ci/circleci: build Your tests passed on CircleCI!
Details
ci/circleci: build_integration Your tests passed on CircleCI!
Details
ci/circleci: build_pf3_docs Your tests passed on CircleCI!
Details
ci/circleci: build_pf4_docs Your tests passed on CircleCI!
Details
ci/circleci: lint Your tests passed on CircleCI!
Details
ci/circleci: test_jest_other Your tests passed on CircleCI!
Details
ci/circleci: test_jest_pf4 Your tests passed on CircleCI!
Details
ci/circleci: upload_docs Your tests passed on CircleCI!
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.