patternfly / patternfly-react

A set of React components for the PatternFly project.
https://react-staging.patternfly.org/
MIT License
773 stars 352 forks source link

Investigate react-select component #2531

Closed LHinson closed 4 years ago

LHinson commented 5 years ago

In discussion with @karelhala, he mentioned how robust the react select component is. This would provide him with a rich set of features for the component, that PatternFly has not yet build.

In this issue, PatternFly is to investigate whether or not we could leverage react-select. It should include detailing any pros and cons in order to determine how to proceed.

This should be done in coordination with core.

dlabaj commented 5 years ago

Took a look at this component and what we currently have for our select component implementation. While this does offer a lot of functionality currently we are going to make the updates requested by consumers to the existing PF4 component. We have had some issues pulling in 3rd party components in the past (tippy and react table) when trying to get them to conform to our PF4 implementations. This also has more features then we currently want to support, and we would prefer not to have support additional functionality that we didn't intend to have.

Hyperkid123 commented 5 years ago

This also has more features then we currently want to support, and we would prefer not to have support additional functionality that we didn't intend to have.

@dlabaj the reason why we are suggesting this is that we are already using react-select and we need these features now. We need async variant, we need create-able variant, custom filtering, etc.

We have had some issues pulling in 3rd party components in the past (tippy and react table) when trying to get them to conform to our PF4 implementations.

While i understand this argument i don't think its valid for this component. Take a look at the react-select docs. There is a ton of customization and you can literally replace default component with PF4 ones and use your css classes and css variables that exist in PF at this moment.

And if you don't want this to be part of your core repo why not make it an separate package as is done with your tables which depends on reactabular.

I think it would be much better solution than having a component that is not usable for most of the use cases we have. There are already two packages with styled react select for PF4 and we are using those instead of react-core select component.

dLobatog commented 5 years ago

Currently on https://github.com/RedHatInsights/compliance-frontend we are using an adapted form of react-select to use PF4 styles and it's definitely fitting our use case - https://github.com/data-driven-forms/react-forms/tree/master/packages/pf4-component-mapper/src/form-fields/select

Before, I had a few issues with the typeahead in https://www.patternfly.org/v4/documentation/react/components/select/ . (Creatable) - Being able to create new objects on demand is not there as far as I can see. Furthermore, I wanted to have a callback every time the user writes something (onInputChange), which I cannot do with the current PF4 select implementation - the event onSelect only triggers when I select an option.

Since there's already a library that's quite popular and devs know how to deal with its implementation, plus there are myriads of examples and documentation online, why not using it and just styling it? It seems that that's possible according to the repo I linked above (data-driven-forms)

It looks like this - as you can see I can easily connect to redux-form and we even auto-create new options if you stop typing for > 2s:

@dlabaj You mentioned

currently we are going to make the updates requested by consumers to the existing PF4 component.

What are these requests? Do they include the two things I mentioned before (Creatable and onInputChange)? If so, why not take advantage of a pre-existing library, that's already adapted to PF4, and well-documented? It's not like Patternfly does not do that already, all of the patternfly-chart charts are eventually rendering a VictoryChart (https://formidable.com/open-source/victory/docs/victory-chart/)

martinpovolny commented 5 years ago

In my perspective @dLobatog and @Hyperkid123 gave enough arguments and examples that cast a shadow on PF's decision in this matter.

Can you revisit this?

martinpovolny commented 5 years ago

@karelhala : what's your view on this?

karelhala commented 5 years ago

dlabaj closed this 19 hours ago

Wow quite a hasty aproach on this critical issue. I expected a bit more discussion on this topic with a few stakeholders and try to discuss features that are aleady implemented in the react select component and see if we could come up with solution to improve PF select component.

While this does offer a lot of functionality currently we are going to make the updates requested by consumers to the existing PF4 component.

As @Hyperkid123 mentioned, we are already requiring these features and because PF select was lacking in them we looked and found react select as a midterm solution and started to love it. The interaction, customization and usage is so simple that we felt like it would be great addition to PF. While some things are resolved (custom filter and custom children) by #2434 it still covers just a small portion of what react select supports. Plus instead of spending few days adapting to react select (it took us a day and a half writing styles and tests - RedHatInsights/frontend-components/pull/93) you are proposing a lot of mandays spent on basically writing similiar component. That doesn't sound like opensource way.

This brings me to

This also has more features then we currently want to support, and we would prefer not to have support additional functionality that we didn't intend to have.

I quite don't understand why having more features is a showstopper. Can you elaborate on it? Isn't it the oposite, a good thing?

ryelo commented 5 years ago

I can understand the hesitation of bringing in another dependency, but could we just hold it in another package similar to react-table and charts?

As @karelhala said, I don't understand why having more features is bad. It is a great component and will be a huge improvement in a lot of frontends on cloud.redhat.com.

If Patternfly doesn't want to roll with it, I'd be happy to meet with Patternfly design to see how we can keep it up to spec in our library and then just iterate on it there instead of here. At the very least, I can go through it and keep it as close to Patternfly as we can. I'd rather not dismiss this component altogether when it provides so much.

janwright73 commented 5 years ago

Discussed this issue with Ryan, Karel and others today - we would like to understand the must have features that react-select provides that are needed by products so that the PF team can estimate time to deliver those features in existing technology. Thanks for the extra dialog and understanding. @dlabaj @kmcfaul - please ping @LHinson or I if you have questions.

karelhala commented 5 years ago

When I opened https://github.com/patternfly/patternfly-react/issues/2211 we were trying to use PF select component and found a lot of issues with it so we looked around and found this really good component. Some of these things are currently used or as planed features.

I want to stress out that we are already using the react-select component and we styled it to look like PF4 select component. It was 2 days effort and we are willing to share our styles, given current API of select in PF4 I don't think that it would require much of a update and we could adapt to it rather easilly.

It's not a small list of features I know and as you guys said you don't want to support really complex component for select. So why not to have a select component that has limited usage and should be used when users don't want any fancy functions of react-select and restyling react-select as different package so consumers can choose it and it will lift a lot of work from your shoulders and allow to free devs on some more crutial components.

Hyperkid123 commented 5 years ago

Probably the most important feature in addition to list that @karelhala just mentioned is in my opinion component customization.

React select offers amazing API for switching the building blocks of the component which allows us to adjust any sub-component exactly as we need.

Once you encounter even the smallest visual adjustment and current PF4 implementation does not support it, we are done and cannot move forward with it.

LHinson commented 5 years ago

@karelhala will you clarify in the list below if it's currently used or planned? and if it's planned, when you plan to adopt it? Thanks in advance for helping us to understand the full story here to help evaluate.

  • direct connection to redux
  • async loading - this one is a huge one, indicate that data are loading (on select and in option container), loading placeholder, etc.
  • creatable values
  • new options by typing them in input - different from creatable values, these options stay in pool of options.
  • auto placement of option container - this is a huge benefit
  • option to rewrite any component
  • input changes directly to app devs
  • custom formatter for options
  • clearable callback
karelhala commented 5 years ago

@LHinson all of these features are currently being used with exception of async loading where we are still trying to figure out what kind of async are we going to use, either onmount load or on type loading.

LHinson commented 5 years ago

Thanks for the additional information @karelhala We're looking into it.

dlabaj commented 5 years ago

@Hyperkid123 and @karelhala Taking another look. Would like to understand what you need the ability customize any components that make up the select component? Is it just the spinner for loading from the server that's in issue #2211? Thanks.

karelhala commented 5 years ago

Would like to understand what you need the ability customize any components that make up the select component?

Well we and probably oher apps in future will receive some variations of select, be it options or different input and other things. Some of these variants will be directly bound to the project and they wouldn't be used outside (I know that PF is trying to sync all designs but these things will occure) or adding them to PF would take some time (designs would have to be revisited, new styles writen and after that react - this could take weeks). The ability to change how the select looks like is awesome and we should leverage that as well. Great examples are custom components for no options and placeholder. We want to have translations in our products and wihout these custom components it would be super hard to implement them correctly.

From top of my head I can think of custom color picker where you'd write the color and it would be selected in color palette, or custom datepicker. Actually this component could serve as datepicker and we could allow custom formatting of date because we can control input and everything.

dgutride commented 5 years ago

@karelhala - we'd like to close this if you are ok with it. Based on the outcome of our investigation and our conversations with the cloud services platform team, we'd like to continue investing in our select component in react-core due to the amount of work already put in and the fact that we are working to address other usability concerns you had. Please let me know if you'd like me to keep this open.

karelhala commented 5 years ago

@dgutride yes, we can close it. If someone requests features to be added to react-core select they'll open new issue. Thank you for the investigation.

ibolton336 commented 4 years ago

Moving to the PF4 select component in its current state is a concern for a few reasons:

ibolton336 commented 4 years ago

@karelhala Added some feedback here. Rather than open a new issue, I was hoping you could re-open this one? Thanks

karelhala commented 4 years ago

@ibolton336 sure we can reopen this one, however wouldn't these issues lost in this quite old issue?

onToggle callback prop is required. This adds additional overhead by forcing the end user to handle the toggle state within the UI.

That is actually by design, every dropdown like component does that in order to allow for more customizable experience. Do you want to close on each select or after user clicks outside? With one unified solution there would be a lot of ifs.

ibolton336 commented 4 years ago

That is actually by design, every dropdown like component does that in order to allow for more customizable experience. Do you want to close on each select or after user clicks outside? With one unified solution there would be a lot of ifs.

I think there should be some sort of default onToggle that handles most users primary use case. Then if the user requires modification, open up the onToggle prop for customization.

kmcfaul commented 4 years ago

@ibolton336 Two of the bullets should be resolvable.

Regarding isPlaceholder- You may leave out the placeholder option of the options list and use the placeholderText property instead to provide a default string (or node) without showing it in the list. The display logic of the select dropdown prefers {selected option display} > {placeholderText} > {isPlaceholder option display}.

Regarding option typing- SelectOption supports a SelectOptionObject type as well as string to allow custom data within options. A SelectOptionObject may contain any structure of data, but must define and instantiate a toString function to let the component display and compare values. Somewhat related to this, custom display of an option is also possible using the children property of SelectOption.

For the other three, here are my thoughts. Pinging @rachael-phillips @tlabaj to chime in here as well. Regarding selections- I agree it's confusing for the single variant. The property serves all select variants, most of which are multiple selection. It would be a breaking change to rename or introduce a replacement property for single select, so it would take more time to bring in, but is possible to do.

Regarding statefulness: To my knowledge, this is by design, component state is minimized if not made stateless. Adding in a simple stateful select may not be a terrible idea though.

Regarding onToggle- As Karel has pointed out, this is by design, matching other similar components like Dropdown.

ibolton336 commented 4 years ago

Regarding onToggle- As Karel has pointed out, this is by design, matching other similar components like Dropdown.

The issue with this approach is that if there are multiple selects within a component, I am required to keep track of multiple toggle methods & boolean values which clutters up the downstream app. If there were a stateful component offering, that would be really useful.

ibolton336 commented 4 years ago

Thanks @kmcfaul for the help. Was able to resolve most of my issues.

tlabaj commented 4 years ago

@kmcfaul @ibolton336 I have opened an issue to at least update the prop description for the 'selections` prop so it is more clear. #3106

dgutride commented 4 years ago

@ibolton336 - circling back around on this - seems like @kmcfaul was able to help you resolve the issues. Do you feel like a new stateful component offering would help with the other pieces? If so, I'd like to recommend we close this issue and open something new to track that request if you are ok with it.

ibolton336 commented 4 years ago

Do you feel like a new stateful component offering would help with the other pieces? If so, I'd like to recommend we close this issue and open something new to track that request if you are ok with it.

Sure, we can close this. Yes that would be great if we could get something stateful in place for the above mentioned reasons. I will go ahead and open an issue @dgutride

stale[bot] commented 4 years ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.