feat(Grid): Add the react-bootstrap grid system #77

sharvit · 2017-11-16T14:37:04Z

Add the react-bootstrap grid components to patternfly-react (Grid, Row, Col, Clearfix)

What:
Add gird-system components from react-bootstrap:

Grid
Row
Col
Clearfix

Why:
To reduce the need to import react-bootstrap package

How:
Export those components from react-bootstrap

Storybook:
https://sharvit.github.io/patternfly-react/?knob-Label=Danger%20Will%20Robinson%21&knob-ShowClearfix=true&selectedKind=Grid&selectedStory=Basic%20Grid&full=0&down=1&left=1&panelRight=0&downPanel=storybooks%2Fstorybook-addon-knobs

priley86 · 2017-11-16T17:36:38Z

thanks @sharvit - I don't see any issue w/ this. We'll be able to convert this repo to SASS pretty soon most likely, but LESS works for now 😄

The only thing I'm noting here now is that our defaultTemplate shows "This pattern does not yet exist in PatternFly." Since PF just uses Bootstrap rows/cols, I'm wondering if we shouldn't update the default template to accept a bootstrapDocumentationLink which renders in place of our This is an existing PatternFly component... and says something like This pattern uses Bootstrap CSS. Refer to the official Bootstrap documentation: {link}. In this case, we could probably link to here:
https://getbootstrap.com/docs/3.3/css/#grid

Thoughts @jgiardino @mturley @LHinson ?

sharvit · 2017-11-16T18:06:08Z

Are you planning to export patternfly scss files as part of the package?
If so, I think that would be really great 👍

It is making sense to add the bootstrap docs urls to the views API.

priley86 · 2017-11-16T18:32:36Z

I believe the preliminary plan is to still consume PF Core SASS from PF Core (patternfly npm), and to consume any PF React specific SASS from patternfly-react, but we are hoping to keep as much SASS in PF Core as possible (more on this in upcoming community meetings). I guess you would prefer to just consume patternfly-react? I'm not sure if this will be intended, but we will have to regroup on this soon...

mturley · 2017-11-16T19:29:13Z

This seems fine to me at first glance, is it really necessary though to make a two-line file for each of the grid components? If they are just imported and exported again in index.js, could we just import from react-bootstrap in index.js?

priley86 · 2017-11-16T23:54:28Z

@mturley @sharvit i'd be ok with removing the two-line files as long as it's consistent. We can open up a subsequent PR to change the others if that is the preference...

mturley · 2017-11-17T03:45:06Z

I didn't realize there was a precedent. That's fine, we can just leave it.

…

On Thu, Nov 16, 2017, 7:02 PM Patrick Riley ***@***.***> wrote: @mturley <https://github.com/mturley> @sharvit <https://github.com/sharvit> i'd be ok with removing the two-line files as long as it's *consistent*. We can open up a subsequent PR to change the others if that is the preference... — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#77 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAxju8Mj3VXnQvHLLojniLkyqEjxrLjWks5s3Ms0gaJpZM4QglZG> .

sharvit · 2017-11-17T16:13:11Z

@mturley @priley86 I prefer to have one file for each component so when I want to look into component I only need to search for a file with the component name.

About the scss, I prefer to consume only one package that can supply for me the components, scss, font, icons and so...
Also being able to use scss with mixins and variables instead of using less is a big plus.

jgiardino · 2017-11-17T16:14:40Z

Thanks for contributing this, @sharvit! This is a great addition!

There were a couple of things I noticed regarding classes. @andresgalante could you also review this one and see if there's anything I missed?

There are some page layouts in the patternfly test pages that include classes on the container or row, for example the Cards page layout does this. Is it possible to add classes to the <Grid>, <Row> and <Col> elements?
All of the page layouts we have in patternfly use the class container-fluid. Is it possible to have this be the default for the <Grid> element assuming that most of the devs would want that and to avoid the potential for them to forget or overlook it? Or at the least, could we update the storybook example to use <Grid fluid=true> so that this is included in the story source code? It might also be nice to enable this attribute as a knob, just to call additional attention to it so that people can see the difference, but I'm open to thoughts on that.

sharvit · 2017-11-17T16:30:14Z

@jgiardino

Yes, those components are very flexible, you can use className with all of them and you can also use bsClass to change the base class (from container to pf-container).
Yes, just add fluid as an attribute:

<Container fluid>
  ...
</Container>

More cool props:
https://react-bootstrap.github.io/components.html#grid-props

priley86 · 2017-11-20T13:41:17Z

@sharvit i think @jgiardino would like to review this some more today. We should be able to introduce Grid, Row and Col throughout after 😸

priley86 · 2017-11-22T06:24:20Z

@sharvit if you can rebase this and clean up other Grid, Row, Col references, I think we can merge this PR next...

sharvit · 2017-11-22T06:43:14Z

@priley86 - Will be done soon 👍

sharvit · 2017-11-22T08:52:51Z

@priley86 - ready to get merged

priley86 · 2017-11-22T12:51:03Z

@sharvit I think @jgiardino is referring to the fact that most if not all PF patterns today are using fluid layout. Maybe we can default this, or at least just update the story to use fluid in all of our examples?

priley86 · 2017-11-22T12:58:23Z

@sharvit @mturley let me know if this PR is blocking anything else... else I would say, let's wait for @jgiardino's feedback. She is back from the holiday break next week...

@sharvit are you able to start working on Forms / Form components in the mean time?
#65

My guess is these will be needed in many places too...

sharvit · 2017-11-22T13:18:33Z

@priley86 - I don't believe it will be smart to make the fluid as a default behavior.
I will update the storybook soon.

It is not blocking me because i can still use the react-bootstrap version and change it easily later on.

@danseethaler - did you had a progress with the forms and the search since our last chat?

jgiardino · 2017-11-22T13:30:32Z

@sharvit @priley86
+1
I'm ok with not defaulting to fluid as long as this attribute is represented in the storybook. My main goal is that developers can use this component and easily implement patternfly page layouts, without having to dig around too much in other sites to find out how to recreate patternfly layouts with these components.

danseethaler · 2017-11-22T17:37:40Z

@sharvit I have worked on the search component but not the patternfly-react context. It's just been in foreman for the RedHat Repos page. We has some discussion this morning about it though and I will probably be working on getting it over to patternfly-react this iteration.

sharvit · 2017-11-22T20:00:48Z

@priley86 @jgiardino
Updated the storybook to use the fluid prop:
https://sharvit.github.io/patternfly-react/?knob-Label=Danger%20Will%20Robinson%21&knob-fluid=true&knob-ShowClearfix=true&selectedKind=Grid&selectedStory=Basic%20Grid&full=0&down=1&left=1&panelRight=0&downPanel=storybooks%2Fstorybook-addon-knobs

I will continue to integrate the basic form components as describe here #65 (comment)

@danseethaler 👍

ohadlevy · 2017-11-23T13:14:12Z

src/Grid/Clearfix.js

@@ -0,0 +1,2 @@
+import { Clearfix } from 'react-bootstrap'


cant you just

export { Clearfix } from 'react-bootstrap'

@ohadlevy I wanted to be consistent with other files like:
https://github.com/patternfly/patternfly-react/blob/master/src/MenuItem/MenuItem.js

Exporting components from react-bootstrap is a common task, i think it should be consistent everywhere.

@ohadlevy @sharvit of course i'm OK with making the two line files one line files. Let's just be consistent...

Feel free to update the others alongside this PR if you can. Otherwise we can do a subsequent PR.

priley86 · 2017-11-25T22:59:19Z

src/Grid/index.js

@@ -0,0 +1,4 @@
+export { default as Grid } from './Grid'


Again, consistency thing here. I haven't been using an individual index.js in each component, but if we decide to, should it be done everywhere? Just please update the PR to make other components consistent.

priley86 · 2017-11-25T23:00:57Z

@sharvit other than the consistency issues, i think this is good to be merged now. I will merge this PR if you can reach a resolution on this.

sharvit · 2017-11-26T09:19:21Z

@priley86 - I feel I should create a new pr to reflect my point of view about the consistency and we can continue to discuss from there.

priley86 · 2017-11-28T00:55:25Z

@sharvit 👍 works great for me. Let's rebase other PRs after.

priley86 · 2017-11-28T14:56:45Z

@sharvit any chance you can pull up this PR soon? I think it will be important to clarify it.

sharvit · 2017-11-28T18:42:34Z

@priley86 the consistency one? I believe I can 👍

priley86 · 2017-11-28T23:32:33Z

yep - would be great to figure this out so I can do less rebases later :)

sharvit · 2017-12-06T16:57:39Z

@priley86
rebased

http://rawgit.com/sharvit/patternfly-react/storybook/Grid/index.html?knob-Label=Danger%20Will%20Robinson%21&knob-fluid=true&selectedKind=Grid&selectedStory=Basic%20Grid&full=0&down=1&left=1&panelRight=0&downPanel=storybooks%2Fstorybook-addon-knobs

Add the react-bootstrap grid components to patternfly-react (Grid, Row, Col, Clearfix)

priley86 · 2017-12-06T18:03:33Z

@sharvit looking good 👍 - I think we are ready to merge here. We can convert all Less to Sass in a future PR.

@jgiardino any further issues here?

jgiardino · 2017-12-06T18:14:45Z

LGTM!

priley86 · 2017-12-06T18:16:29Z

@jgiardino 💯 ... and we can say it was you if anything breaks 🤣 😆 ??

…nfly#77) * fix(package): move package to all npm scripts internally * add server.js and change start command

priley86 mentioned this pull request Nov 21, 2017

feat(list-group): adds the pf list-group pattern #74

Merged

sharvit force-pushed the feature/page-layout branch from aa1fba7 to ecf95b1 Compare November 22, 2017 08:47

sharvit force-pushed the feature/page-layout branch from ecf95b1 to 7afc149 Compare November 22, 2017 19:51

ohadlevy reviewed Nov 23, 2017

View reviewed changes

priley86 reviewed Nov 25, 2017

View reviewed changes

priley86 mentioned this pull request Nov 30, 2017

feat(buttongroup): add the pf buttongroup #90

Merged

jgiardino added the review label Dec 5, 2017

sharvit force-pushed the feature/page-layout branch from 7afc149 to c55517c Compare December 6, 2017 16:54

feat(Grid): Add the react-bootstrap grid system

b1bc843

Add the react-bootstrap grid components to patternfly-react (Grid, Row, Col, Clearfix)

sharvit force-pushed the feature/page-layout branch from c55517c to b1bc843 Compare December 6, 2017 16:59

priley86 approved these changes Dec 6, 2017

View reviewed changes

priley86 merged commit 30409e1 into patternfly:master Dec 6, 2017

jgiardino removed the review label Dec 6, 2017

sharvit deleted the feature/page-layout branch December 6, 2017 19:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(Grid): Add the react-bootstrap grid system #77

feat(Grid): Add the react-bootstrap grid system #77

sharvit commented Nov 16, 2017 •

edited

Loading

priley86 commented Nov 16, 2017

sharvit commented Nov 16, 2017

priley86 commented Nov 16, 2017 •

edited

Loading

mturley commented Nov 16, 2017

priley86 commented Nov 16, 2017

mturley commented Nov 17, 2017 via email

sharvit commented Nov 17, 2017 •

edited

Loading

jgiardino commented Nov 17, 2017

sharvit commented Nov 17, 2017 •

edited

Loading

priley86 commented Nov 20, 2017

priley86 commented Nov 22, 2017

sharvit commented Nov 22, 2017

sharvit commented Nov 22, 2017

priley86 commented Nov 22, 2017

priley86 commented Nov 22, 2017

sharvit commented Nov 22, 2017

jgiardino commented Nov 22, 2017

danseethaler commented Nov 22, 2017

sharvit commented Nov 22, 2017

ohadlevy Nov 23, 2017

sharvit Nov 23, 2017

priley86 Nov 25, 2017 •

edited

Loading

priley86 Nov 25, 2017 •

edited

Loading

priley86 commented Nov 25, 2017

sharvit commented Nov 26, 2017

priley86 commented Nov 28, 2017

priley86 commented Nov 28, 2017

sharvit commented Nov 28, 2017

priley86 commented Nov 28, 2017 •

edited

Loading

sharvit commented Dec 6, 2017

priley86 commented Dec 6, 2017

jgiardino commented Dec 6, 2017

priley86 commented Dec 6, 2017

feat(Grid): Add the react-bootstrap grid system #77

feat(Grid): Add the react-bootstrap grid system #77

Conversation

sharvit commented Nov 16, 2017 • edited Loading

priley86 commented Nov 16, 2017

sharvit commented Nov 16, 2017

priley86 commented Nov 16, 2017 • edited Loading

mturley commented Nov 16, 2017

priley86 commented Nov 16, 2017

mturley commented Nov 17, 2017 via email

sharvit commented Nov 17, 2017 • edited Loading

jgiardino commented Nov 17, 2017

sharvit commented Nov 17, 2017 • edited Loading

priley86 commented Nov 20, 2017

priley86 commented Nov 22, 2017

sharvit commented Nov 22, 2017

sharvit commented Nov 22, 2017

priley86 commented Nov 22, 2017

priley86 commented Nov 22, 2017

sharvit commented Nov 22, 2017

jgiardino commented Nov 22, 2017

danseethaler commented Nov 22, 2017

sharvit commented Nov 22, 2017

ohadlevy Nov 23, 2017

Choose a reason for hiding this comment

sharvit Nov 23, 2017

Choose a reason for hiding this comment

priley86 Nov 25, 2017 • edited Loading

Choose a reason for hiding this comment

priley86 Nov 25, 2017 • edited Loading

Choose a reason for hiding this comment

priley86 commented Nov 25, 2017

sharvit commented Nov 26, 2017

priley86 commented Nov 28, 2017

priley86 commented Nov 28, 2017

sharvit commented Nov 28, 2017

priley86 commented Nov 28, 2017 • edited Loading

sharvit commented Dec 6, 2017

priley86 commented Dec 6, 2017

jgiardino commented Dec 6, 2017

priley86 commented Dec 6, 2017

sharvit commented Nov 16, 2017 •

edited

Loading

priley86 commented Nov 16, 2017 •

edited

Loading

sharvit commented Nov 17, 2017 •

edited

Loading

sharvit commented Nov 17, 2017 •

edited

Loading

priley86 Nov 25, 2017 •

edited

Loading

priley86 Nov 25, 2017 •

edited

Loading

priley86 commented Nov 28, 2017 •

edited

Loading