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

Component - Select #54

Closed
kahowell opened this issue Sep 29, 2017 · 15 comments
Closed

Component - Select #54

kahowell opened this issue Sep 29, 2017 · 15 comments

Comments

@kahowell
Copy link

Currently we are porting a UI for subscription-management from the cockpit project. In our efforts, we needed to utilize a select component. The simplest implementation was to use bootstrap-select. For reference, here's what we're doing at the moment: candlepin/subscription-manager#1709 (we're using jQuery internally to style the select, but it feels wrong).

The cockpit developers have a mostly complete implementation in pure react (i.e. no jQuery): https://github.com/cockpit-project/cockpit/blob/162aa4a6892459e5b611b36432cfef9aea9bfcb6/pkg/lib/cockpit-components-select.jsx This was not easy to integrate, given that the components in cockpit are coupled, and cockpit doesn't currently provide a node package.

Reference: http://www.patternfly.org/pattern-library/widgets/#bootstrap-select

It would be great if this were implemented properly in patternfly-react. Is there a plan to implement this?

@waldenraines waldenraines self-assigned this Sep 29, 2017
@waldenraines
Copy link

It would be great if this were implemented properly in patternfly-react. Is there a plan to implement this?

I actually ported bootstrap-select to react several months ago and haven't touched it since. Let me dust it off and open a PR.

@waldenraines
Copy link

waldenraines commented Sep 29, 2017

Since this is just a react version of bootstrap-select with patternfly classes would something like https://github.com/tjwebb/react-bootstrap-select work?

@kahowell
Copy link
Author

kahowell commented Sep 29, 2017

would something like https://github.com/tjwebb/react-bootstrap-select work?

Yeah, that would work for us. We want to use whatever the upstream patternfly/patternfly-react community uses as much as possible. Are you thinking that patternfly-react could simply add that as a dependency and document appropriately?

Edit: typo

@waldenraines
Copy link

Yeah, that would work for us. We want to use whatever the upstream patternfly/patternfly-react community uses using as much as possible. Are you thinking that patternfly-react could simply add that as a dependency and document appropriately?

I'm not sure how we should approach this for patternfly-react, do we want to include that library in patternfly-react or just recommend it as an approach for selects?

I know that patternfly currently contains bootstrap-select but I don't know whether that is their intention longer term after they pull out the jQuery bits into another project.

@priley86 any thoughts on this? I can see the benefits of both approaches.

@priley86
Copy link
Member

priley86 commented Oct 2, 2017

@waldenraines I will raise this at today's React repo design review. I think react-bootstrap-select may work given its similarity but we'll have to contrast with what's in PF core today and discuss differences.

I would propose incorporating this lib as a dependency in pf-react and standardizing its design there like you suggested. It's our hope to keep this repo as pure React components (non jquery).

@waldenraines
Copy link

I would propose incorporating this lib as a dependency in pf-react and standardizing its design there like you suggested. It's our hope to keep this repo as pure React components (non jquery)

I'm confused. I think if we want to avoid jQuery in patternfly-react we will have to write this ourselves or customize something like react-select to look like the bootstrap-select that is used in patternfly core. Unfortunately react-bootstrap-select is really just a react wrapper for the jQuery implementation.

I'm personally in favor of not including jQuery in this project so I'd lean towards the approach mentioned above.

@priley86
Copy link
Member

priley86 commented Oct 2, 2017

@waldenraines sorry, did not realize react-bootstrap-select was just a jquery wrapper. For that reason, I do not think this is a good candidate to introduce as a dependency. We'd like to take a similar approach in pf-react as we've taken in patternfly-ng (and only implement or consume pure React components implemented as ES6 modules). Similarly, patternfly-ng consumes pure Ng2+ components (example: ngx-bootstrap).

My vote would be to write our own pure React components that are Bootstrap styled, or find a pure React port elsewhere upstream. If you've already written one of these @waldenraines, feel free to start the contribution (I'm happy to review anything you'd like to propose).

I was also looking at another project recently called downshift for examples on ARIA compliant autocomplete/dropdown/select/combobox React components. We may be able to reuse some code from this project (or at least compare/contrast accessibility principles implemented). However, these are not Bootstrap styled, so if you have something else in mind for now please suggest it!

@priley86
Copy link
Member

priley86 commented Oct 3, 2017

@waldenraines react-select looks promising to me. It appears we'll need to import some additional CSS to make it work, but I think we'll be customizing that CSS anyways (so I'd be fine with customizing that CSS in this project for now).

I've already added PF LESS to a separate Storybook I've been playing with by extending Storybook's webpack config (so that we can import our own LESS into Storybook). I'd propose we do something similar to import CSS (with css-loader or less-loader).
https://github.com/priley86/patternfly-webcomponents-next/blob/master/frameworks/angular/storybook/config.js

@jgiardino @waldenraines Do you want me to add a PR for consuming PF LESS first? I could have something ready today or tomorrow on this, and then @waldenraines could start looking to add react-select?

@jgiardino can you possibly take a look at react-select from a design perspective and see what we'd need to customize in the CSS?

@waldenraines
Copy link

@jgiardino @waldenraines Do you want me to add a PR for consuming PF LESS first? I could have something ready today or tomorrow on this, and then @waldenraines could start looking to add react-select?

The react-select implementation I created was just a wrapper around jQuery as well but we could look at doing another one using react-select. I think consuming patternfly-less would be a good thing to do regardless,

@priley86
Copy link
Member

priley86 commented Oct 3, 2017

ok @waldenraines - I've chatted with @jgiardino on this. I think we are ready to consume PF LESS in this project. This should allow further customization and enable us to write our own LESS and extend PF LESS.

Feel free to build on top of this PR or customize it or let me know if you'd like to merge it. I am fine with changing file structures/folder organization if desired, but this gives the basic idea of how we can do this with Storybook:
#57

Please note that less folder is not included in .npmignore should someone need to consume it downstream.

I'd propose we convert the react-select CSS to LESS inside of less directory, or just include it directly from patternfly-react.less and extend it where needed.

i.e.:
less/patternfly-react.less:

@import './patternfly.less';
@import select.less

less/select.less:

@import 'react-select/dist/react-select.css';
... (our custom CSS/LESS extensions below)

@priley86
Copy link
Member

priley86 commented Oct 11, 2017

@waldenraines Please note that #57 should enable consuming a third party component such as react-select. Please feel free to proceed with this should you be ready :)

@jgiardino jgiardino added next and removed p1 labels Dec 15, 2017
@waldenraines waldenraines removed their assignment Dec 15, 2017
@priley86 priley86 self-assigned this Jan 10, 2018
@priley86
Copy link
Member

Update: I am currently reviewing the styles applied for react-bootstrap select and comparing our existing jquery bootstrap-select in PF Core. We should be able to handle most simple use cases of the select with a minor bit of CSS. Will follow up with an upstream PR into Core shortly and reference that issue here.

@priley86
Copy link
Member

I believe we can target the Downshift component for selects/multiselects as well. That work was started here:
#190

Moving this to backlog for now...

@priley86 priley86 removed their assignment Feb 12, 2018
@priley86
Copy link
Member

priley86 commented Mar 16, 2018

i noted in mIQ/v2v that we still needed to wrap jquery boostrap-select typeahead/select to match the existing mIQ design most closely. It would be nice to formalize the select/typeahead pattern in PF-React in the future w/ this issue and formally vet the pattern in PatternFly (ensuring a11y, design, forward facing React API support).
https://github.com/priley86/miq_v2v_ui_plugin/pull/148

jeff-phillips-18 added a commit to jeff-phillips-18/patternfly-react that referenced this issue Jun 6, 2018
affects: patternfly-react

Really cool stuff... you will like it

ISSUES CLOSED: patternfly#54
@stale
Copy link

stale bot commented Jun 6, 2019

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

@stale stale bot added the wontfix label Jun 6, 2019
@stale stale bot closed this as completed Jun 20, 2019
HarikrishnanBalagopal pushed a commit to HarikrishnanBalagopal/patternfly-react that referenced this issue Sep 29, 2021
chore(readme): update readme with helpful information
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants