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

Use div in text columns and create responsive grid. #3438

Merged
merged 2 commits into from Jan 4, 2018

Conversation

Projects
None yet
5 participants
@samikeijonen
Contributor

samikeijonen commented Nov 11, 2017

Description

Update text columns block markup and create responsive Grid. See issues #2908. Note that depending on content width 3-4 columns blocks might not have 3-4 columns.

We can always force 4 columns using media queries but I wanted to hear opinions first using fluid responsive Grid.

Columns could get really narrow and unreadable if we force them to be in 4 columns.

Screenshots (jpeg or gifs if applicable):

Screenshot of Twenty Seventeen (current PR):
4 columns fluid grid

Screenshot of Twenty Seventeen if forcing to 4 columns:
4 columns forced

How Has This Been Tested?

  • Running npm test was successful.
  • Testing with default themes and random .org themes.

Types of changes

  • Change <section> to <div>.
  • Change to fluid Grid.

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows has proper inline documentation.
@StaggerLeee

This comment has been minimized.

Show comment
Hide comment
@StaggerLeee

StaggerLeee Nov 13, 2017

Many times comes to my mind WP core would make it very easy for themself, save lot of time, to use Bootstrap library for back-end.
Does Bootstrap licence allows it ?

This problem with columns is practicaly solved with Bootstrap, and for all screen sizes. And very popular library. Not only magic with columns, but all other magic too.

Other plugins could use it to. For instance ACF, Pods, CMB2, etc... and tweaking fields layout, Metaboxes in back-end. Right now they use some limited hard tweaking for it.

Start by little steps and in some distant future convert whole back-end to Bootstrap. Replace all HTML tables with it.

StaggerLeee commented Nov 13, 2017

Many times comes to my mind WP core would make it very easy for themself, save lot of time, to use Bootstrap library for back-end.
Does Bootstrap licence allows it ?

This problem with columns is practicaly solved with Bootstrap, and for all screen sizes. And very popular library. Not only magic with columns, but all other magic too.

Other plugins could use it to. For instance ACF, Pods, CMB2, etc... and tweaking fields layout, Metaboxes in back-end. Right now they use some limited hard tweaking for it.

Start by little steps and in some distant future convert whole back-end to Bootstrap. Replace all HTML tables with it.

@samikeijonen

This comment has been minimized.

Show comment
Hide comment
@samikeijonen

samikeijonen Nov 13, 2017

Contributor

I personally don't see benefits using Bootstrap or any CSS framework for Grids. You can do that for couple of lines of CSS.

But unified Design System for WP admin would be nice. But that's out of scope from this PR :)

Contributor

samikeijonen commented Nov 13, 2017

I personally don't see benefits using Bootstrap or any CSS framework for Grids. You can do that for couple of lines of CSS.

But unified Design System for WP admin would be nice. But that's out of scope from this PR :)

@afercia

This comment has been minimized.

Show comment
Hide comment
@afercia

afercia Dec 12, 2017

Contributor

Using a div instead of improperly using a section make sense to me. Not an expert about @supports (grid-area: auto). @mtias @jasmussen would be nice to have some feedback, when you have a chance.

Contributor

afercia commented Dec 12, 2017

Using a div instead of improperly using a section make sense to me. Not an expert about @supports (grid-area: auto). @mtias @jasmussen would be nice to have some feedback, when you have a chance.

@samikeijonen

This comment has been minimized.

Show comment
Hide comment
@samikeijonen

samikeijonen Dec 12, 2017

Contributor

Couple of question to think about:

  • On narrow screens do we want one column layout?
  • On larger screens do we always force let's say 4 columns when user sets 4 columns?
  • If we do, how we decide the breakpoint without knowing how the content width works in a theme?
  • Do we set another breakpoint where items could be in 2 columns first, and then 4 columns?
  • Or do we let the columns just be fluid without breakpoints like in this PR?
Contributor

samikeijonen commented Dec 12, 2017

Couple of question to think about:

  • On narrow screens do we want one column layout?
  • On larger screens do we always force let's say 4 columns when user sets 4 columns?
  • If we do, how we decide the breakpoint without knowing how the content width works in a theme?
  • Do we set another breakpoint where items could be in 2 columns first, and then 4 columns?
  • Or do we let the columns just be fluid without breakpoints like in this PR?
@jasmussen

This comment has been minimized.

Show comment
Hide comment
@jasmussen

jasmussen Dec 13, 2017

Contributor

So, two avenues of answers to this one.

First: the "Text Columns" block is sort of a one-off. As we implemented it, the chief reason for doing so was to demonstrate the power of what Gutenberg can do with layouts, even in its current state. But because of all the questions asked in this thread (all of which have already also been asked on the Columns ticket, #219), this might be the type of block that doesn't actually ship with Gutenberg initially.

Secondly, the thing is — depending on the implementation of columns — we might want the UI for nested blocks to be in place first in #428, and once work properly starts on #219 we might not want a separate block for "Text Columns" at all, but depending on the approach perhaps a generic column container. This container could then have options for columns as well as responsiveness.

This is also where CSS Grid can come into play, and CC: @mor10 because he's mentioned that a number of times.

All of this is not to say that we can't or shouldn't make improvements to text-columns, just that it's perhaps worth starting to think about what form those improvements should take, before we spend too much time optimizing for something that may not exist indefinitely. If it's just a quick markup change, that's probably super fine.

Contributor

jasmussen commented Dec 13, 2017

So, two avenues of answers to this one.

First: the "Text Columns" block is sort of a one-off. As we implemented it, the chief reason for doing so was to demonstrate the power of what Gutenberg can do with layouts, even in its current state. But because of all the questions asked in this thread (all of which have already also been asked on the Columns ticket, #219), this might be the type of block that doesn't actually ship with Gutenberg initially.

Secondly, the thing is — depending on the implementation of columns — we might want the UI for nested blocks to be in place first in #428, and once work properly starts on #219 we might not want a separate block for "Text Columns" at all, but depending on the approach perhaps a generic column container. This container could then have options for columns as well as responsiveness.

This is also where CSS Grid can come into play, and CC: @mor10 because he's mentioned that a number of times.

All of this is not to say that we can't or shouldn't make improvements to text-columns, just that it's perhaps worth starting to think about what form those improvements should take, before we spend too much time optimizing for something that may not exist indefinitely. If it's just a quick markup change, that's probably super fine.

@afercia

This comment has been minimized.

Show comment
Hide comment
@afercia

afercia Dec 13, 2017

Contributor

@jasmussen thanks, postponing any CSS change sounds good to me. @samikeijonen when you have a chance, mind updating your PR to have just the markup changes? 🙂

Contributor

afercia commented Dec 13, 2017

@jasmussen thanks, postponing any CSS change sounds good to me. @samikeijonen when you have a chance, mind updating your PR to have just the markup changes? 🙂

@samikeijonen

This comment has been minimized.

Show comment
Hide comment
@samikeijonen

samikeijonen Dec 13, 2017

Contributor

I'll update or create fresh PR at some point. I still think my questions is good base for deciding how the columns/nested blocks could work.

The CSS part is the easy part, using Grid or flexbox, or both.

Contributor

samikeijonen commented Dec 13, 2017

I'll update or create fresh PR at some point. I still think my questions is good base for deciding how the columns/nested blocks could work.

The CSS part is the easy part, using Grid or flexbox, or both.

@codecov

This comment has been minimized.

Show comment
Hide comment
@codecov

codecov bot Dec 31, 2017

Codecov Report

Merging #3438 into master will decrease coverage by 5.37%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##           master   #3438      +/-   ##
=========================================
- Coverage   39.17%   33.8%   -5.38%     
=========================================
  Files         299     249      -50     
  Lines        7025    6713     -312     
  Branches     1292    1207      -85     
=========================================
- Hits         2752    2269     -483     
- Misses       3588    3754     +166     
- Partials      685     690       +5
Impacted Files Coverage Δ
blocks/library/text-columns/index.js 33.33% <ø> (+4.76%) ⬆️
components/panel/row.js 0% <0%> (-100%) ⬇️
components/notice/index.js 0% <0%> (-100%) ⬇️
blocks/block-controls/index.js 0% <0%> (-100%) ⬇️
blocks/url-input/button.js 0% <0%> (-100%) ⬇️
blocks/color-palette/index.js 0% <0%> (-90.91%) ⬇️
blocks/alignment-toolbar/index.js 16.66% <0%> (-83.34%) ⬇️
editor/components/post-last-revision/check.js 0% <0%> (-75%) ⬇️
editor/components/post-taxonomies/index.js 28.57% <0%> (-57.15%) ⬇️
editor/components/document-outline/index.js 17.85% <0%> (-57.15%) ⬇️
... and 273 more

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 976f8c6...b65c74d. Read the comment docs.

codecov bot commented Dec 31, 2017

Codecov Report

Merging #3438 into master will decrease coverage by 5.37%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##           master   #3438      +/-   ##
=========================================
- Coverage   39.17%   33.8%   -5.38%     
=========================================
  Files         299     249      -50     
  Lines        7025    6713     -312     
  Branches     1292    1207      -85     
=========================================
- Hits         2752    2269     -483     
- Misses       3588    3754     +166     
- Partials      685     690       +5
Impacted Files Coverage Δ
blocks/library/text-columns/index.js 33.33% <ø> (+4.76%) ⬆️
components/panel/row.js 0% <0%> (-100%) ⬇️
components/notice/index.js 0% <0%> (-100%) ⬇️
blocks/block-controls/index.js 0% <0%> (-100%) ⬇️
blocks/url-input/button.js 0% <0%> (-100%) ⬇️
blocks/color-palette/index.js 0% <0%> (-90.91%) ⬇️
blocks/alignment-toolbar/index.js 16.66% <0%> (-83.34%) ⬇️
editor/components/post-last-revision/check.js 0% <0%> (-75%) ⬇️
editor/components/post-taxonomies/index.js 28.57% <0%> (-57.15%) ⬇️
editor/components/document-outline/index.js 17.85% <0%> (-57.15%) ⬇️
... and 273 more

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 976f8c6...b65c74d. Read the comment docs.

@samikeijonen

This comment has been minimized.

Show comment
Hide comment
@samikeijonen

samikeijonen Jan 1, 2018

Contributor

Removed responsive grid (which still feels super weird to me).

Contributor

samikeijonen commented Jan 1, 2018

Removed responsive grid (which still feels super weird to me).

@jasmussen

This comment has been minimized.

Show comment
Hide comment
@jasmussen

jasmussen Jan 3, 2018

Contributor

Seems like this is ready for a final review?

Contributor

jasmussen commented Jan 3, 2018

Seems like this is ready for a final review?

@samikeijonen

This comment has been minimized.

Show comment
Hide comment
@samikeijonen

samikeijonen Jan 3, 2018

Contributor

Yep, should be just change to <div>. Maybe we should do deprecate version for this too?

Contributor

samikeijonen commented Jan 3, 2018

Yep, should be just change to <div>. Maybe we should do deprecate version for this too?

@@ -102,20 +102,20 @@ registerBlockType( 'core/text-columns', {
/>
</div>
) }
</section>,
</div>,
];
},
save( { attributes } ) {

This comment has been minimized.

@youknowriad

youknowriad Jan 4, 2018

Contributor

Since we change the save I guess we need a deprecated version here unless we're ok showing a warning for old text-columns blocks considering this block as not a block that is used that much? cc @jasmussen

@youknowriad

youknowriad Jan 4, 2018

Contributor

Since we change the save I guess we need a deprecated version here unless we're ok showing a warning for old text-columns blocks considering this block as not a block that is used that much? cc @jasmussen

This comment has been minimized.

@jasmussen

jasmussen Jan 4, 2018

Contributor

In general I think all these sorts of semi-breaking changes we should make as fast as we can in this phase, and not worry too much. We have great tools to help users "fix" the blocks when they are updated, so I think we should just do it.

If we are going to do something different once nested blocks come into play, then we'd need to do further changes regardless.

@jasmussen

jasmussen Jan 4, 2018

Contributor

In general I think all these sorts of semi-breaking changes we should make as fast as we can in this phase, and not worry too much. We have great tools to help users "fix" the blocks when they are updated, so I think we should just do it.

If we are going to do something different once nested blocks come into play, then we'd need to do further changes regardless.

@youknowriad youknowriad merged commit d01a464 into WordPress:master Jan 4, 2018

2 checks passed

codecov/project 39.17% remains the same compared to 976f8c6
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@samikeijonen samikeijonen deleted the samikeijonen:update/text-columns branch Jan 17, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment