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

Columns Block: Make responsive, make editor and theme match #10541

Merged
merged 7 commits into from Oct 18, 2018

Conversation

@jasmussen
Contributor

jasmussen commented Oct 12, 2018

This PR does a number of things.

  1. It fixes #7818 by making sure the editor view and theme view are the same, dimension wise. It also adds some margin betweeen columns.
  2. It fixes #6048 by adding basic responsiveness. Essentially mobile has 1 column, beyond mobile has 2, beyond "medium" (782 breakpoint) we show whatever the user has set it to, i.e. 3 or up to 6.
  3. It refactors the margins and paddings of the columns block to "level the playing field" between editor and theme, basically making the dimensions the same. This involved making the column (singular) block essentially lose its visual block padding and footprint.

Editor:

screenshot 2018-10-12 at 10 52 25

Theme:

screenshot 2018-10-12 at 10 53 16

Dimensions:

columns

Responsive:

responsive

There is some overlap with the column blocks due to the fact that this PR makes them more or less lose their footprint, which makes the hover outlines overlap at times. Perhaps the longer term fix here is to make those blocks actual pass-through blocks so they don't have hover outlines, or can be selected. But in the mean time, this PR intends to make the columns block ship-ready for 5.0.

Please test it thoroughly and decide if you agree!

@jasmussen jasmussen added this to the 4.1 milestone Oct 12, 2018

@jasmussen jasmussen self-assigned this Oct 12, 2018

@jasmussen jasmussen requested review from mtias, aduth and WordPress/gutenberg-core Oct 12, 2018

@GlennMartin1

This comment has been minimized.

GlennMartin1 commented Oct 13, 2018

But in the mean time, this PR intends to make the columns block ship-ready for 5.0.

Perhaps this PR would be the appropriate time/place to remove the Beta designation?

i.e. change Columns (beta) to Columns.

@gziolo

This comment has been minimized.

Member

gziolo commented Oct 16, 2018

This is what I see in this branch:

6 columns:
screen shot 2018-10-16 at 14 15 33

2 empty columns:
screen shot 2018-10-16 at 14 16 28

@jasmussen

This comment has been minimized.

Contributor

jasmussen commented Oct 16, 2018

Oh the placeholder text needs to be elided. Applying overflow: hidden; and text-overflow: ellipsis; would probably address that. Though this is the same in master, isn't it?

@gziolo

This comment has been minimized.

Member

gziolo commented Oct 16, 2018

I will double check later, I just leave my observations since we removed beta I'm doing more detailed review :)

@gziolo

This comment has been minimized.

Member

gziolo commented Oct 16, 2018

I can confirm that the issues I shared are regressions. This is how they look in master:

6 columns:
screen shot 2018-10-16 at 14 44 58

2 empty columns:
screen shot 2018-10-16 at 14 43 30

@gziolo gziolo requested a review from sarahmonster Oct 16, 2018

@gziolo

This comment has been minimized.

Member

gziolo commented Oct 16, 2018

Some other things that don't quite work as expected:

Sidebar overlaps columns (6 columns case):
screen shot 2018-10-16 at 15 16 00

Buttons (Upload, Media Library, inserter) are not usable on medium viewport:

Edit: that's actually not an issue, it's a feature, you need to click on the block first, sorry about confusion :(

It is not possible to scroll screen on small viewport (buttons are not actionable, too):

Edit: I can't reproduce anymore after rebasing with master branch.

@gziolo

See my previous comments

@sarahmonster

This comment has been minimized.

Contributor

sarahmonster commented Oct 16, 2018

I'm getting totally lost trying to sift through the columns here and to suss out what CSS is targeting what. I don't think this is a regression—I've gotten painfully lost every time I've tried to test the columns block, but it's making it super difficult for me to provide helpful feedback and/or suggestions here.

I'm seeing a lot of overlapping elements:

screenshot 2018-10-16 13 36 20

screenshot 2018-10-16 13 36 42

screenshot 2018-10-16 13 36 54

screenshot 2018-10-16 13 43 50

I'm not sure if this is intended behaviour, but it's contributing to the overall feeling of lost-ness.

Hiding all the placeholder content (so both "Write your story" and "Add text or type / to add content") would help clean things up a bit, since this seems to pop up all over the place and often in unexpected spots.

I can't select the ellipsis menu for any of the individual columns at the right, or those columns that are below another column block, and I have a lot of difficulty selecting the parent column block:

2018-10-16 13 50 46

What's the rationale behind making each individual column selectable, as well as the parent column? Is this an extensability thing, or is it just so you can apply CSS classes to the columns individually? Can we simplify by removing this ability? Would it make sense to include a hairline outline of the columns, so that people could explicitly see the column structure at work and better understand where each column begins and ends?

Using an embed block seems to break the columns out of the editor window:

screenshot 2018-10-16 14 02 47

Columns don't span the full width of the screen if the content in them is short—this may also be by design, but I would have expected a full-width sort of deal here:

screenshot 2018-10-16 14 36 19

I'm not certain how much of this is helpful or specific to this PR at this stage, but I can test a bit more later today to see if I can't suss out some of these issues in more detail. Offhand, I wonder if using grid instead of flexbox might make some of these behaviours a bit more consistent and reliable.

@ZebulanStanphill

This comment has been minimized.

Contributor

ZebulanStanphill commented Oct 16, 2018

This issue exists on master, but it looks like the min-width value of some things are causing issues with the column widths. For example, this embed placeholder has a min-width that causes its column to become bigger than it should be:

image

@sarahmonster

This comment has been minimized.

Contributor

sarahmonster commented Oct 16, 2018

Yep, that's what happening here. Since it isn't a regression it's probably not strictly important to fix here. I tried fixing it by applying an overflow: hidden rule to the container column, but that meant that the toolbar for blocks would then get cut off. Apparently there's a way to pop things outside an overflow container, but it relies on relative positioning, which the toolbar doesn't use, and seems like it might be at best a pretty fragile solution.

@tofumatt

This comment has been minimized.

Member

tofumatt commented Oct 16, 2018

What's the rationale behind making each individual column selectable, as well as the parent column? Is this an extensibility thing, or is it just so you can apply CSS classes to the columns individually?

As the parent column is also a block, it needs to be selectable for accessibility reasons (keyboard nav) but also so you can select it to change settings.

Some of these seem less regressions since they work on master. There is a lot of stuff to test here. 😒

@sarahmonster showed me (in-person) thus far the only fix that consistently works needs to be applied high enough in the DOM that it clips the block toolbar 😢

@jasmussen

This comment has been minimized.

Contributor

jasmussen commented Oct 16, 2018

The tricky thing here is that UI freeze is tomorrow. If we can get this in, we can fix bugs later. If we don't get this in, I'm not sure.

The css here is complex, and the block as a whole needs love in phase 2. However for basic things it's hugely valuable.

If we feel this PR improves what's in master and doesn't introduce new issues, I feel it's worth merging as an iterative improvement and then track anything missing.

@mtias any thoughts?

@tofumatt

This comment has been minimized.

Member

tofumatt commented Oct 16, 2018

I am usually for improvements that still have bugs but don't cause regressions. 😄

Columns block in general though needs more work I think. I'd rather leave a beta label and :shipit:

@ZebulanStanphill

This comment has been minimized.

Contributor

ZebulanStanphill commented Oct 16, 2018

The Columns block is, for non-technical-users, almost entirely useless with responsive columns, so I say merge this if there are no regressions but leave the (Beta) label until the block becomes less buggy.

What exactly does "UI freeze" mean?

@sarahmonster

This comment has been minimized.

Contributor

sarahmonster commented Oct 16, 2018

Responsiveness = totally an improvement. I'm on board with "ship it now, fix it later".

@jasmussen

This comment has been minimized.

Contributor

jasmussen commented Oct 17, 2018

This one actually is responsive. But I'm okay with adding back the beta label.

@karmatosed

This comment has been minimized.

Member

karmatosed commented Oct 17, 2018

I would +1 to adding in with beta, or whatever it takes get this in as responsive. It's asked for a lot in feedback and feels like a win.

@gziolo gziolo force-pushed the try/columns-fixes branch from dcccc1f to cfba143 Oct 18, 2018

@gziolo gziolo self-assigned this Oct 18, 2018

@gziolo

This comment has been minimized.

Member

gziolo commented Oct 18, 2018

Added 1ca6139

Before
screen shot 2018-10-16 at 14 16 28

After:
screen shot 2018-10-18 at 13 09 58

@gziolo

This comment has been minimized.

Member

gziolo commented Oct 18, 2018

Added: 632dcfe

Before

Editor:
screen shot 2018-10-18 at 13 14 28

Frontend:
screen shot 2018-10-18 at 13 13 33

After

Editor:
screen shot 2018-10-18 at 13 28 24

Frontend:
screen shot 2018-10-18 at 13 25 16

@gziolo

gziolo approved these changes Oct 18, 2018

@gziolo

This comment has been minimized.

Member

gziolo commented Oct 18, 2018

I added some tweaks to the PR. There are still some outstanding issues on the mobile view. However they aren't related to the responsive mode at all. See screencast recorded on master branch which has the same issue:

columns

@gziolo gziolo merged commit 4ba27ff into master Oct 18, 2018

2 checks passed

codecov/project 49.5% remains the same compared to 73ce8fd
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@gziolo gziolo deleted the try/columns-fixes branch Oct 18, 2018

@jasmussen

This comment has been minimized.

Contributor

jasmussen commented Oct 18, 2018

♥️♥️♥️♥️

@paaljoachim

This comment has been minimized.

paaljoachim commented Oct 22, 2018

https://make.wordpress.org/test/
Test the responsiveness of the Columns block—still designated beta but improvements have been made, try just a few columns for now. (10541)

Responsiveness looks good!

I have a few other minor things that I noticed...
Adding a column block the focus goes to paragraph block inside the columns block and not the column block setting.
The user has to nudge the mouse cursor around to find the border of the column block to be able to select the column block setting. Should the column block because it is a container have a thicker border to give a signal of a difference in block? As blocks that have another block inside could have thicker borders. Visually one can see the difference by just looking at the thickness of the border outlines.

Inserting a block inside the column is not done so on the left side as it the usual approach but on the right side. It would be good to keep it consistent.

antpb added a commit to antpb/gutenberg that referenced this pull request Oct 26, 2018

Columns Block: Make responsive, make editor and theme match (WordPres…
…s#10541)

* Add basic responsiveness.

* Refactor columns metrics. Level the playing field in editor and frontend.

* Add space between colums.

Fixes WordPress#7818.
Fixes WordPress#6048.

* Remove "beta" designation from columns block.

* Columns block: Fix column width when editing

* Column block: Improve padding for the first and last item in a row

* Tests: Rename Columns block name also in e2e tests
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment