feat(grid): base implementation #93

bmdalex · 2018-08-15T14:43:20Z

Grid

This PR will introduce the Grid component with the following props:

columns=[number|string]
rows=[number|string]

The Grid will specify the layout of the elements it encapsulates as children or in the content prop.

TODO

API Proposal

`columns`

We can specify a certain amount of columns:

or the explicit columns for a grid:

<Grid columns="7">
  <Image fluid src="img1.png" />
  <Image fluid src="img2.png" />
  <Image fluid src="img3.png" />
  ...
</Grid>

and

<Grid columns="repeat(3, 1fr) 2fr 2fr 110px 14rem 50px 20%">
  <Image fluid src="img1.png" />
  <Image fluid src="img2.png" />
  <Image fluid src="img3.png" />
  ...
</Grid>

generate:

<div class="ui-grid a b c d e f k">
  <img src="img1.png" class="ui-image h i b j" />
  <img src="img2.png" class="ui-image h i b j" />
  <img src="img3.png" class="ui-image h i b j" />
  ...
</div>

`rows`

We can specify a certain amount of rows:

or the explicit rows for a grid:

<Grid rows="2">
  <Image fluid src="img1.png" />
  <Image fluid src="img2.png" />
  <Image fluid src="img3.png" />
  ...
</Grid>

and

<Grid rows="2fr repeat(2, 1fr)">
  <Image fluid src="img1.png" />
  <Image fluid src="img2.png" />
  <Image fluid src="img3.png" />
  ...
</Grid>

generate:

<div class="ui-grid a b c d e f k">
  <img src="img1.png" class="ui-image h i b j" />
  <img src="img2.png" class="ui-image h i b j" />
  <img src="img3.png" class="ui-image h i b j" />
  ...
</div>

codecov · 2018-08-15T15:05:24Z

Codecov Report

Merging #93 into master will increase coverage by 0.31%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master      #93      +/-   ##
==========================================
+ Coverage   86.52%   86.84%   +0.31%     
==========================================
  Files          39       41       +2     
  Lines         668      684      +16     
  Branches      102      103       +1     
==========================================
+ Hits          578      594      +16     
  Misses         87       87              
  Partials        3        3

Impacted Files	Coverage Δ
src/components/Grid/Grid.tsx	`100% <100%> (ø)`
src/index.ts	`100% <100%> (ø)`	⬆️
src/components/Grid/index.tsx	`100% <100%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 00375ed...dc7e057. Read the comment docs.

kuzhelov · 2018-08-15T17:23:01Z

sorry in advance, maybe I am missing something. Could you, please, suggest if there is any way for me as a client of this functionality to explicitly provide division on rows - in a way similar to Semantic UI does:

<Grid >
  <Grid.Row>
     .. elements here..
  </Grid.Row>
  <Grid.Row>
     .. another elements here..
  </Grid.Row>
</Grid>

gaui · 2018-08-15T17:26:38Z

What are those extra letter classes? a b c d e f k

bmdalex · 2018-08-15T17:40:46Z

@kuzhelov no, this new Grid version does not have the concept of rows and columns as these are controlled with props; see proposal #11.

Therefore, all we have are grid items (that can be any element as we do not have any constraints there); in the future we're probably going to add a GridItem component for controlling styles of individual items

bmdalex · 2018-08-15T17:41:32Z

@gaui Fella related classes that add the styles internally; if it's confusing I'm gonna delete them from the example as they are irrelevant there

bmdalex · 2018-08-15T17:43:20Z

@levithomason removed itemSize from the API and adjusted default gap and padding; this is ready for review now and can probably be merged as is

levithomason

Per chat, let's only merge rows / columns for now. These props hit our use cases.

The itemSize can be discussed in more detail later on subsequent PRs. It has some implications that will need collaboratively worked out before merging.

levithomason · 2018-08-15T17:48:10Z

docs/src/examples/components/Grid/Types/GridExample.shorthand.tsx

+  <div>
+    <Grid content={images} />
+  </div>
+)


Wrapping div is not required, this can be reduced to:

const GridExample = () => <Grid content={images} />

levithomason · 2018-08-15T17:50:12Z

docs/src/examples/components/Grid/Types/GridExample.shorthand.tsx

+
+const images = imageNames.map((name, index) => (
+  <Image key={`${name}-${index}`} fluid src={`public/images/avatar/large/${name}`} />
+))


Given the amount of code required to abstract and loop over the images, it would be less code and easier for users to scan if we inline the images:

<Image fluid src="public/images/avatar/large/ade.jpg" /> <Image fluid src="public/images/avatar/large/chris.jpg" /> <Image fluid src="public/images/avatar/large/christian.jpg" /> <Image fluid src="public/images/avatar/large/daniel.jpg" /> <Image fluid src="public/images/avatar/large/elliot.jpg" /> <Image fluid src="public/images/avatar/large/elyse.png" /> <Image fluid src="public/images/avatar/large/helen.jpg" /> <Image fluid src="public/images/avatar/large/jenny.jpg" /> <Image fluid src="public/images/avatar/large/joe.jpg" /> <Image fluid src="public/images/avatar/large/justen.jpg" />

@levithomason
I have 20+ images in other grid examples to make it clear to users how the grid looks like ; duplicating 20+ lines in other places doesn't really make sense; do we want to change it only for these 2 examples (GridExample.shorthand.tsx and GridExample.tsx) ?

The criteria for examples is concise and readable. We want them to fit on the minimal number of lines so that the can be easily viewed inline in the doc site. In cases where abstractions significantly reduce the amount code, then they are helpful and we should use them.

In the example here, given any number of images, the abstracted code will require more lines of code since each image path in the array already requires a line of code. Any lines required for abstraction are not needed.

In examples where the array of images is used more than once, then we'll certainly saves lines by abstracting. We should use an abstraction for those. However, you might consider removing the map code and just creating an array of images once:

const images = [ <Image key="ade" fluid src="public/images/avatar/large/ade.jpg" />, <Image key="chris" fluid src="public/images/avatar/large/chris.jpg" />, <Image key="christian" fluid src="public/images/avatar/large/christian.jpg" />, <Image key="daniel" fluid src="public/images/avatar/large/daniel.jpg" />, <Image key="elliot" fluid src="public/images/avatar/large/elliot.jpg" />, <Image key="elyse" fluid src="public/images/avatar/large/elyse.png" />, <Image key="helen" fluid src="public/images/avatar/large/helen.jpg" />, <Image key="jenny" fluid src="public/images/avatar/large/jenny.jpg" />, <Image key="joe" fluid src="public/images/avatar/large/joe.jpg" />, <Image key="justen" fluid src="public/images/avatar/large/justen.jpg" />, ]

levithomason · 2018-08-15T17:57:52Z

src/components/Grid/Grid.tsx

+        PropTypes.arrayOf(customPropTypes.itemShorthand),
+        customPropTypes.itemShorthand,
+      ]),
+    ]),


In the case of a Grid component, I'm not sure content makes sense. Perhaps here we only support children?

Why? There is no processing to be done on the content by Grid. It is also an "invisible" component in that it only formats children, but doesn't render its own subcomponents.

Thoughts?

I agree with the reasons, but we have following things preventing us to do so:

all components should support shorthand version; removing content breaks that pattern basically; it'd be the first component without shorthand support, right?

we were discussing introducing GridItem component for controlling individual items; maybe it'll make sense at that point

Yes, good points. We probably should not make those types of decisions in this PR. Let's keep it as is and we can discuss it in another. This would be a framework level design change.

levithomason · 2018-08-15T17:59:53Z

src/themes/teams/componentStyles.ts

 export { default as List } from './components/List/listStyles'
 export { default as ListItem } from './components/List/listItemStyles'

 export { default as Menu } from './components/Menu/menuStyles'
-


Let's avoid unnecessary whitespace changes. It will make the PR smaller and prevent us from flip flopping on code style updates over time.

A good way to propose and implement style fixes is to make a PR introducing auto fixable lint rules or prettier config. This way, code style is automatic and enforced.

I was just removing some random new lines, at least grouping by components with same letter; ok, I'll add my change only

bmdalex · 2018-08-15T18:43:01Z

@levithomason I see some visual tests failing but I cannot login to this screener thingie to see what's up

levithomason · 2018-08-16T02:27:49Z

Send me the email you'd like to use on Screener privately and I'll add you.

bmdalex · 2018-08-17T01:46:18Z

@levithomason solved the screener issue with help from Miro. Everything is passing and I think I addressed all comments

bmdalex requested review from dvdzkwsk, kuzhelov, levithomason and mnajdova as code owners August 15, 2018 14:43

bmdalex force-pushed the feat/grid-v1 branch 2 times, most recently from 67329a7 to ae870b9 Compare August 15, 2018 15:00

bmdalex force-pushed the feat/grid-v1 branch from ae870b9 to c13c5c6 Compare August 15, 2018 15:05

bmdalex mentioned this pull request Aug 15, 2018

[WIP] feat(grid): base implementation #15

Closed

11 tasks

bmdalex force-pushed the feat/grid-v1 branch from c13c5c6 to c415cbe Compare August 15, 2018 17:34

bmdalex changed the title ~~feat(grid): base implementation~~ [READY FOR REVIEW] feat(grid): base implementation Aug 15, 2018

levithomason suggested changes Aug 15, 2018

View reviewed changes

levithomason reviewed Aug 15, 2018

View reviewed changes

levithomason added the 🚀 ready for review label Aug 15, 2018

levithomason changed the title ~~[READY FOR REVIEW] feat(grid): base implementation~~ feat(grid): base implementation Aug 15, 2018

levithomason added needs author feedback Author's opinion is asked and removed 🚀 ready for review labels Aug 15, 2018

bmdalex force-pushed the feat/grid-v1 branch from c415cbe to dc5bdaa Compare August 15, 2018 18:32

bmdalex added 🚀 ready for review and removed needs author feedback Author's opinion is asked labels Aug 15, 2018

bmdalex force-pushed the feat/grid-v1 branch from 736266a to 08e3a73 Compare August 16, 2018 09:04

bmdalex force-pushed the feat/grid-v1 branch 2 times, most recently from fa9b88a to 888c4e5 Compare August 17, 2018 01:44

bmdalex force-pushed the feat/grid-v1 branch 3 times, most recently from 7c3ccac to c71be9c Compare August 20, 2018 12:23

EUROPE\albuliga and others added 2 commits August 20, 2018 16:15

feat(grid): base implementation

622f2b0

addressed PR comments

dc7e057

bmdalex force-pushed the feat/grid-v1 branch from c71be9c to dc7e057 Compare August 20, 2018 14:16

levithomason approved these changes Aug 20, 2018

View reviewed changes

levithomason merged commit ceb2d94 into master Aug 20, 2018

levithomason deleted the feat/grid-v1 branch August 20, 2018 21:46

bmdalex mentioned this pull request Jun 3, 2019

docs(Examples): fix Grid variables to fix gaps in Firefox #1428

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(grid): base implementation #93

feat(grid): base implementation #93

bmdalex commented Aug 15, 2018 •

edited

codecov bot commented Aug 15, 2018 •

edited

kuzhelov commented Aug 15, 2018

gaui commented Aug 15, 2018

bmdalex commented Aug 15, 2018

bmdalex commented Aug 15, 2018

bmdalex commented Aug 15, 2018

levithomason left a comment

levithomason Aug 15, 2018

bmdalex Aug 15, 2018

levithomason Aug 15, 2018

bmdalex Aug 15, 2018

levithomason Aug 15, 2018

bmdalex Aug 15, 2018

levithomason Aug 15, 2018

bmdalex Aug 15, 2018

levithomason Aug 15, 2018

levithomason Aug 15, 2018

bmdalex Aug 15, 2018

bmdalex Aug 15, 2018

bmdalex commented Aug 15, 2018

levithomason commented Aug 16, 2018

bmdalex commented Aug 17, 2018

feat(grid): base implementation #93

feat(grid): base implementation #93

Conversation

bmdalex commented Aug 15, 2018 • edited

Grid

TODO

API Proposal

columns

rows

codecov bot commented Aug 15, 2018 • edited

Codecov Report

kuzhelov commented Aug 15, 2018

gaui commented Aug 15, 2018

bmdalex commented Aug 15, 2018

bmdalex commented Aug 15, 2018

bmdalex commented Aug 15, 2018

levithomason left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bmdalex commented Aug 15, 2018

levithomason commented Aug 16, 2018

bmdalex commented Aug 17, 2018

bmdalex commented Aug 15, 2018 •

edited

`columns`

`rows`

codecov bot commented Aug 15, 2018 •

edited