feat(Grid): Add grid support for multiple device visibility breakpoints

[layershifter]

Fixes #1024.

[codecov-io]

Codecov Report

Merging #1347 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #1347   +/-   ##
=======================================
  Coverage   99.74%   99.74%           
=======================================
  Files         141      141           
  Lines        2354     2354           
=======================================
  Hits         2348     2348           
  Misses          6        6

Impacted Files	Coverage Δ
src/collections/Grid/GridRow.js	100% <ø> (ø)	✅
src/collections/Grid/GridColumn.js	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 3c8e536...4c39736. Read the comment docs.

[levithomason]

❤️

[layershifter]

@levithomason I've added examples to docs, so you can pull and give me the review when you will have time.

<Grid.Row onlyLarger='mobile' /> // => <div class="tablet computer only"></div>
<Grid.Row onlyLarger='tablet' /> // => <div class="computer only"></div>
<Grid.Row onlyLarger='computer' /> // => <div class="large screen widescreen only"></div>
<Grid.Row onlyLarger='largeScreen' /> // => <div class="widescreen only"></div>

<Grid.Row onlySmaller='computer' /> // => <div class="mobile tablet only"></div>
<Grid.Row onlySmaller='tablet' /> // => <div class="mobile only"></div>

If we go to SUI CSS, you will undertand my logic there 😄

We can't do behaviour below, because computer will be shown on large screen and wide screen.

<Grid.Row onlySmaller='largeScreen' /> // => <div class="mobile tablet computer only"></div>

---

Resuming. After this I do not think it's a good idea, it looks very confusing.

[levithomason]

I've plotted out the * only sizes. Dashed lines are where the classes are visible. The x indicates visibility termination. Finally -> and <- indicate visibility continues forever.

                             768         992        1200        1920
                              |           |           |           |
mobile             <----------|x          |           |           |
mobile tablet      <----------|-----------|x          |           |
tablet                       x|-----------|x          |           |
computer                      |          x|-----------|-----------|---------->
large screen                  |           |          x|-----------|---------->
widescreen                    |           |           |          x|---------->
                              |           |           |           |

I want to include something like this in the docs along with some more explanation on the classes as they are pretty confusing without going through this exercise. However, now that we have this chart, we can map the classes.

[layershifter]

I'll take a short break to rethink this 🤔 However, the ideas with table is awesome.

[layershifter]

I think we need to change our initial idea about onlyLarger / onlySmaller.

// will include `tablet`, `computer`
// `largeScreen` and `wideScreen` is unnecessary there
<Grid.Row onlyLarger='mobile' />
<Grid.Row only='tablet computer' />

// will include `computer`
// `largeScreen` and `wideScreen` is unnecessary there
<Grid.Row onlyLarger='tablet' />
<Grid.Row only='computer' />

// will include `largeScreen` and `widescreen`
<Grid.Row onlyLarger='computer' />
<Grid.Row only='largeScreen widescreen' />

// will include only `widescreen`, so it's unnecessary
<Grid.Row onlyLarger='largeScreen' />
<Grid.Row only='widescreen' />

// will include nothing
<Grid.Row onlyLarger='widecreen' />

// we need to include there `computer` and `largeScreen`, but it doesn't make sence
<Grid.Row onlySmaller='widescreen' />

// we need to include there `computer`, but it doesn't make sence
<Grid.Row onlySmaller='largeScreen' />

// will include `mobile` and `tablet`
<Grid.Row onlySmaller='computer' />
<Grid.Row only='mobile tablet' />

// will include `mobile`
<Grid.Row onlySmaller='tablet' />
<Grid.Row only='mobile' />

// will include nothing
<Grid.Row onlySmaller='mobile' />
<Grid.Row only='mobile' />

---

After this small analysis we can see, that there are only three variants that makes sence:

<Grid.Row only='mobile tablet' />
<Grid.Row only='tablet computer' />
<Grid.Row only='largeScreen widescreen' />

My proposal is to add them as independent props:

<Grid.Row only='mobile tablet' /> // === <Grid.Row onlyCompact />
<Grid.Row only='tablet computer' /> // === <Grid.Row withoutMobile />
<Grid.Row only='largeScreen widescreen' /> // === <Grid.Row onlyBigScreen />

It will be much cleaner I think.

---

Also, I think we need to standartize largeScreen and widescreen values to largeScreen and wideScreen.

@levithomason I'm glad to hear your feedback.

[levithomason]

Super helpful analysis, thank you. I think given the inconsistencies, we should just not implement onlyLarger nor onlySmaller nor any other version of this feature. The new support for multiple only values covers these cases very nicely without adding too much API surface area. Let's just include the updates to the only prop to allow multiple values.

Also, I think we need to standartize largeScreen and widescreen values to largeScreen and wideScreen.

widescreen is a valid single word noun, see here. However, large screen is an adjective describing the size of a screen. It is unfortunate that the spacing and camelCasing are not consistent, however, it is correct.

---

On another note, I believe when I checked this PR previously the classNames were generated like so:

only='mobile tablet'
// => "mobile tablet only"

However, grid.css defines these classes as order dependent classNames where only is included for each size. Let's confirm we're producing this className string from this prop:

only='mobile tablet'
// => "only mobile only tablet"

[layershifter]

It's ready 😄 I've added breaking label because large screen was renamed to largeScreen.

[levithomason]

What do you think about keeping large screen and shipping this as a non-breaking change? If we feel strongly about refactoring before 1.0 then we can always still do that and ship a breaking change later.

We do also have other multiple word prop values that are not camel cased, such as:

<Input labelPosition='right corner' />
<Sidebar animation='scale down' />
<Popup position='bottom center' />

It may make sense to keep the values as two words.

[layershifter]

I see only one way how we can keep large screen:

<Grid.Row only='tablet' />
<Grid.Row only=['tablet', 'computer'] />
<Grid.Row only=['large screen', 'widescreen'] />

And it makes sence.

[layershifter]

@levithomason Can you give a feedback there?

[levithomason]

We could a bit of parsing on the string:

const only = 'mobile tablet computer large screen widescreen'

only
  .replace('large screen', 'large-screen')
  .split(' ')
  .map(x => `only ${x.replace('-', ' ')}`)
  .join(' ')

// => 'only mobile only tablet only computer only large screen only widescreen'

[levithomason]

This looks great, thanks!

[levithomason]

Released in [email protected].

[Samiam519]

Can you update the docs with the proper syntax to account for multiple device visibility? It currently isn't clear based on this issue or #1024 which is the proper way to implement "computer and up"

[brianespinosa]

@Samiam519 please open a ticket if there is an issue with documentation. Commenting on a PR that was merged three years ago isn't going to help anyone.