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

Add text input group demo #6581

Merged
merged 13 commits into from
Nov 12, 2021

Conversation

wise-king-sullyman
Copy link
Contributor

What: Closes #6393

Additional issues: patternfly/patternfly-design#1088

Interaction proposal:
image

@patternfly-build
Copy link
Contributor

patternfly-build commented Nov 11, 2021

Copy link
Contributor

@nicolethoen nicolethoen left a comment

Choose a reason for hiding this comment

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

This LOOKS great!!!

Some interaction issues, though.

I cannot select the key = Name. when I try to click it, it actually populates it with the key = Cluster. And if I type Name: none of the values appear as options.

I think it'll be important to be sure that if I user wanted to interact with this, using only the keyboard - they should be able to. If they wanted to ignore the dropdown completely that should be an option. And if they wanted to use the keyboard to select a key or value, they should be able to.

Copy link
Member

@mcarrano mcarrano left a comment

Choose a reason for hiding this comment

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

This looks great @wise-king-sullyman ! Just a couple of questions/comments:

  • The way that the chip gets cut off when it overflows feels odd. I'm talking about this:

Screen Shot 2021-11-11 at 2 25 37 PMBut unfortunately I don't have a better suggestion. Anyone have any ideas?

  • I understand why this is a demo for Text input group. And while that's technically correct, I doubt anyone would look for it there. Would we move it or link to it with a more descriptive title? What do you think @nicolethoen ?

@wise-king-sullyman
Copy link
Contributor Author

@mcarrano an interaction question/suggestion was brought up by @nicolethoen in a conversation on slack:

Should the user be able to create chips by hitting the enter key even if they are not a valid key-value pair?

This doesn't quite line up with my interpretation of the interaction proposal, but per @nicolethoen:

I think this pattern will be used for running searches on tables - and people are going to want to use this both as an attr/value search and as your standard google search (on a dataset)

Is this functionality that you'd like to see added to this demo?

@nicolethoen
Copy link
Contributor

  • I understand why this is a demo for Text input group. And while that's technically correct, I doubt anyone would look for it there. Would we move it or link to it with a more descriptive title? What do you think @nicolethoen ?

@mcarrano I am always in favor of more explanation.
@wise-king-sullyman It'd probably be super useful to place a sentence or two as a comment before any functions you define within the example (unless they are super straight forward).

Additionally, adding a few sentences before the demo to explain some of the interaction decisions you main or intended use cases you imagine this demo could satisfy would be helpful.

bug occurred when using the mouse to select a menu item after filtering
the menu items with the text input.
@mcarrano
Copy link
Member

@wise-king-sullyman @nicolethoen regarding the question below...

Should the user be able to create chips by hitting the enter key even if they are not a valid key-value pair?

The intended use case for this that I was modeling from ACM constrained the input to predefined attribute-value pairs. So you would not be able to create a chip with an unknown value. That said. there is probably another use case that is more like an auto-complete search where the user is offered a list of suggestions but can also enter any value from the keyboard and hit Enter to create a chip. Would it be possible to create a second demo that models this? I see these as different use cases and it would demonstrate the flexibility of this approach.

Copy link
Contributor

@mcoker mcoker left a comment

Choose a reason for hiding this comment

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

LGTM. Found a bug with the empty <TextInputGroupUtilities> element creating extra space that we can probably fix in core - patternfly/patternfly#4514

@wise-king-sullyman
Copy link
Contributor Author

@mcarrano spinning up a second demo for that functionality is totally possible, though since code freeze was supposed to have happened already I'd like to do so in a new ticket next sprint if that's alright with you.

@mcarrano
Copy link
Member

That's fine @wise-king-sullyman . If you can open a new issue and tag me, I'll make sure it gets into the plan for the next milestone. Thanks.

added support for text only key selection and focusing into the menu for
arrow key based menu navigation.
Copy link
Contributor

@nicolethoen nicolethoen left a comment

Choose a reason for hiding this comment

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

Awesome job!!

Copy link
Contributor

@kmcfaul kmcfaul left a comment

Choose a reason for hiding this comment

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

lgtm!

@kmcfaul kmcfaul merged commit 79532e3 into patternfly:main Nov 12, 2021
@wise-king-sullyman wise-king-sullyman deleted the add-text-input-group-demo branch November 12, 2021 14:07
@patternfly-build
Copy link
Contributor

Your changes have been released in:

  • eslint-plugin-patternfly-react@4.13.1
  • @patternfly/react-catalog-view-extension@4.25.1
  • @patternfly/react-charts@6.27.1
  • @patternfly/react-code-editor@4.15.1
  • @patternfly/react-console@4.25.1
  • @patternfly/react-core@4.174.1
  • @patternfly/react-docs@5.35.1
  • @patternfly/react-icons@4.25.1
  • @patternfly/react-inline-edit-extension@4.19.1
  • demo-app-ts@4.134.1
  • @patternfly/react-integration@4.136.1
  • @patternfly/react-log-viewer@4.19.1
  • @patternfly/react-styles@4.24.1
  • @patternfly/react-table@4.43.1
  • @patternfly/react-tokens@4.26.1
  • @patternfly/react-topology@4.21.1
  • @patternfly/react-virtualized-extension@4.21.1
  • transformer-cjs-imports@4.12.1

Thanks for your contribution! 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Demonstrate key-value filtering
7 participants