FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

feat(Select): add creatable and new features by kmcfaul · Pull Request #2820 · patternfly/patternfly-react · GitHub

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

Merged
dlabaj merged 4 commits into
patternfly:masterfrom
kmcfaul:select-creatable
Sep 6, 2019
Merged

feat(Select): add creatable and new features#2820
dlabaj merged 4 commits into
patternfly:masterfrom
kmcfaul:select-creatable

Conversation

kmcfaul commented Sep 3, 2019

Copy link
Copy Markdown
Contributor

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

Refer to issue: #2665

Copy link
Copy Markdown
Collaborator

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

mcarrano left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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 commented Sep 4, 2019

Copy link
Copy Markdown
Contributor Author

@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 self-requested a review September 4, 2019 20:52
mcarrano previously approved these changes Sep 4, 2019

mcarrano left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Looks good. Thanks @kmcfaul

dlabaj left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

A few comments. Otherwise looks good. Thanks.

jschuler commented Sep 5, 2019

Copy link
Copy Markdown
Contributor

I'm getting the browser autofill popup, should be turned off?

Copy link
Copy Markdown
Member

How is isCreatable different from onCreateOption?

When I have onCreateOption selected I can't create an option:

Copy link
Copy Markdown
Member

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?

Copy link
Copy Markdown
Member

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

kmcfaul commented Sep 5, 2019

Copy link
Copy Markdown
Contributor Author

@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 commented Sep 5, 2019

Copy link
Copy Markdown
Contributor

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

kmcfaul commented Sep 5, 2019

Copy link
Copy Markdown
Contributor Author

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

Copy link
Copy Markdown
Member

I see autocomplete in Chrome @kmcfaul @jschuler

jschuler commented Sep 5, 2019

Copy link
Copy Markdown
Contributor

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

kmcfaul commented Sep 5, 2019

Copy link
Copy Markdown
Contributor Author

I do have it enabled.

jschuler commented Sep 5, 2019

Copy link
Copy Markdown
Contributor

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

kmcfaul commented Sep 5, 2019

Copy link
Copy Markdown
Contributor Author

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

jschuler commented Sep 5, 2019

Copy link
Copy Markdown
Contributor

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

mcarrano commented Sep 5, 2019

Copy link
Copy Markdown
Member

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 commented Sep 5, 2019

Copy link
Copy Markdown
Contributor Author

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 commented Sep 5, 2019

Copy link
Copy Markdown
Contributor

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

tlabaj left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

LGTM

christiemolloy left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

LGTM

dlabaj merged commit f16e0f3 into patternfly:master Sep 6, 2019
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
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.

7 participants


Back | FazBrowse Home | New Git URL