Skip to content
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

Refactor responsive behavior to better deal with nested .alignwide and .alignfull blocks. #701

Merged

Conversation

@allancole
Copy link
Contributor

allancole commented Oct 1, 2019

This change refactors the width logic to better support nested blocks with varying .alignwide and .alignfull utility classes. This is necessary to retain consistency with the default behavior between the Gutenberg editor and the Twenty Twenty frontend. See #676 for a more detailed description.

Editor Before After
image image image (Couldn’t figure out how to make a full screenshot of the editor here) image image

To Do:

  • Make sure the theme’s editor styles match default Gutenberg behaviors for all blocks that support nesting
  • Test with all blocks that support nesting
  • Clean up code

Related Issues
Fixes: #676
Fixes: #965

@allancole

This comment has been minimized.

Copy link
Contributor Author

allancole commented Oct 1, 2019

This one should be a lot cleaner @andersnoren ;-)

allancole added 3 commits Oct 1, 2019
@andersnoren

This comment has been minimized.

Copy link
Contributor

andersnoren commented Oct 2, 2019

@allancole Really sorry for being slow on checking this one – I'm hoping to get to it this evening. Tomorrow morning at the latest.

@allancole

This comment has been minimized.

Copy link
Contributor Author

allancole commented Oct 2, 2019

No worries at all, @andersnoren. It’s actually still a work in progress so feel free to point out anything you find thats out of place when you can 👍.

@carolinan carolinan added the WIP label Oct 3, 2019
@andersnoren

This comment has been minimized.

Copy link
Contributor

andersnoren commented Oct 3, 2019

@allancole I've taken a look now, and I definitely agree that the alignment classes shouldn't break out of the constraints set by any parent elements. There's also a lot of stuff to fix with the alignment classes in the block editor. I have an ever expanding list of adjustments I need to get to this weekend.

A concern I have with this approach for the alignment classes is that elements nested in a alignwide or alignfull element still adhere to the main entry-content width, rather than using the available space in the parent element. For example, this centered title in a alignwide group block:

image

Is displayed like this:

image

I definitely think users should be able to match the width of the centered content column by nesting blocks within a alignwide or alignfull block (which they currently aren't in the Twenty Twenty master branch, cough), but I think the default should be for all elements to expand to fill the available space in the parent block. IE if you add a heading or paragraph to a alignwide group element, the heading or the paragraph also ends up 1200px wide.

I tried a solution for getting the nesting to work correctly within the current structure (with a max-width on the .entry-content element), with which the alignwide and alignfull blocks only break out of their container when they are direct descendants of the .entry-content wrapper. (It only also includes a lot of other block related fixes that I'm going to break out into separate PRs.) I agree that a structure where the content container has a 100% width feels more appropriate for the .alignwide and .alignfull classes, but the structure feels vulnerable to me when there isn't a container with a fixed width keeping the elements in place.

You can find the branch with my tests here: https://github.com/andersnoren/twentytwenty/tree/test/adjust-nested-alignments

@allancole

This comment has been minimized.

Copy link
Contributor Author

allancole commented Oct 7, 2019

A concern I have with this approach for the alignment classes is that elements nested in a alignwide or alignfull element still adhere to the main entry-content width, rather than using the available space in the parent element. For example, this centered title in a alignwide group block:

Yup, I definitely understand your concern here and ran into the same thing myself :-). Sometimes I want the Header Block to just be wide or even full, but the editor does not support it in the UI currently. So for me the actual question becomes “How do we make a Header Block wide/full when the editor does not support it in the UI?”

One way, is to nest the Header Block in a Group Block as you described above but I view this as a happy accident that only works because the Group Block + nesting didn’t exist in core from the very beginning. It’s tough to follow at first, but allowing the Group Block to influence nested blocks will create a couple of subtle problems for users: (1) How will they know that a Group Block has an effect on the width of its children blocks (especially for blocks that don’t support any width controls)? And (2) how would a user roll it back to normal if they actually prefer the .aligndefault width (shorter sentences with the small font-size might not always look great wide). In my opinion, the Group Block should not assume what users may want to do with it’s nested/children Blocks. Instead we should allow users to make the decision on the child Blocks themselves, so that they (and we) can always be explicit about how a block appears.

The good thing though its actually super easy to make a Header Block stretch wide/full-width without wrapping it in a Group Block. You can simply add the .alignwide utility class to the “Additional CSS Class(es)” part of the Header Block panel. Here’s a screenshot of it in action:

image

Editor:
image

This solution isn’t easily discoverable, but it’s much closer to giving users explicit control over a Blocks appearance and it can be baked into a Theme Template just like any other setting :-) . It’s counter-intuitive but when you think about it, we wanna make sure users can do pretty much anything they want in the editor. So in most cases, we want to avoid changing the appearance of a given Block because of its context or because it happens to be nested or next to a particular Block. If anything, I think this is a really good case to help convince core to add .alignwide/full support to the editor UI for headings and paragraph (and maybe lists too). If that already existed, then the Group Block solution wouldn’t be necessary anyway. If they do eventually add it, the Headings Block with a wide utility class will already work properly without having to submit a new issue for it. But stretched Headings/Paragraph Blocks in nested Group Blocks would sort of be locked out of the improvement and likely require a bug fix.

I agree that a structure where the content container has a 100% width feels more appropriate for the .alignwide and .alignfull classes, but the structure feels vulnerable to me when there isn't a container with a fixed width keeping the elements in place.

Yeah, this is a good point and does feel a bit vulnerable for the reasons you described bit its actually really strict. The CSS says that any and everything that is not .alignwide or .alignfull should strictly follow the max-width set by the theme and be horizontally centered. The one area I’ve found where vulnerabilities come up are for floated alignleft and alignright blocks. Even in this PR those aren’t working very well yet. But having done it this a way few times in other themes, its just a matter of figuring out the right math to get the positioning right responsively.

I hope I’m not coming across to heavy handed with this response. I’m happy to make this work however you prefer. Just want to make sure all other options and concerns were on the table before a final decision is made 👍

@yannickiki

This comment has been minimized.

Copy link
Contributor

yannickiki commented Nov 1, 2019

@andersnoren I fully agree with @allancole, the width of a block shouldn't change because it is inside or outside a group. The user should be able to easily reproduce a simple layout built outside a group (heading, text, image) inside a group. Inserting another group inside a group is not a good solution imo.

Currently, when you set a group to be full-width, default width blocks inside are broken:

  • Paragraph and headings are fullwidth thus unreadable.
  • Blocks that support aligments (image, columns, cover...) are all fullwidth even if the alignment is set to default

I think that when nested inside a full-width group:

  • paragraph, headings, list... and blocks with default alignment should be set to 58rem
  • wide-width blocks should be set to 120rem
  • full-width blocks should go full-width (protected by the group side paddings, that should be set to 4rem on desktop and 2rem on mobile to match the site layout)
@allancole

This comment has been minimized.

Copy link
Contributor Author

allancole commented Nov 5, 2019

@yannickiki, this is still a work-in-progress but I added some changes that get it a little closer to completion. Mostly just need to make sure that the padding on the frontend match the padding in the editor. Once that’s done I’ll ping you for review and hopefully get this merged :-)

@allancole

This comment has been minimized.

Copy link
Contributor Author

allancole commented Nov 5, 2019

@yannickiki, I think this is good to go now. Check it out and let me know if you notice anything off 👍

@yannickiki

This comment has been minimized.

Copy link
Contributor

yannickiki commented Nov 5, 2019

@allancole Nice! I just took a look, here a few things:

I’ll continue to check it.

allancole added 2 commits Nov 7, 2019
allancole added 4 commits Nov 7, 2019
…make author bio width match original design.
…pdate default marging for non-wide/full alignment blocks.
…o ensure expected appreance in nested blocks.
…o ensure expected appreance in nested blocks in the editor.
@allancole

This comment has been minimized.

Copy link
Contributor Author

allancole commented Nov 7, 2019

I think this is mostly working now but I’m having trouble following some of the margin/padding logic in a few areas. In particular, I’m not sure that it makes sense to have the padding that comes with a group block .wp-block-group.has-background { padding: 40px; } increase when that block is full width .wp-block-group.alignfull.has-background { padding: 80px 40px; }. I’m assuming this is to achieve a certain page design, but it feels unexpected when that kind of blocks is nested.

I went ahead and made it so that the varying paddings only happen on top-level group blocks. The effect is that you get this:

Before After
Notice the varying heights in both the magenta and the dark tan areas. Here, only the dark tan colors have varying heights which allows for the original design to stay intact.
image image

If this checks out, I think this PR might be ready to merge :-)

@yannickiki

This comment has been minimized.

Copy link
Contributor

yannickiki commented Nov 8, 2019

Hi @allancole

1) On the front-end, there are issues with .alignleft and .alignright for pullquotes, embed, image...

a) Outside a group (direct children of entry-content), this is what I see with a left-aligned pullquote:
desktop

The max-width should be set to 26rem. Removing the max-width: calc((100% - 58rem) / 2 - 4rem); at the lines below seems to fix the width issue:
https://github.com/allancole/twentytwenty/blob/fix/nested-alignwidth-blocks-two/style.css#L5614
https://github.com/allancole/twentytwenty/blob/fix/nested-alignwidth-blocks-two/style.css#L5622

After removing these lines, it looks good on desktop but there are still some issues on devices smaller than 1220px where the quote is outside the screen. For example at 1024px:

ipad

b) Inside a groupalignleft and alignright are misaligned because they don't get the same css as when they are direct children of entry-content.


2) There are still vertical margin values which differ from the original design

3) Concerning the full width group, the side padding should be set to 4rem and not 6rem to match the other elements side padding (header, footer, wide blocks are protected from the screen edges by 4rem, full width Cover block has a 4rem side spacing...)


4) There is an issue with paragraphs inside .comment-content that have a width of calc(100% - 4rem); instead of 100%


comment

Don't hesitate to ping if you need testing 🙂

@pattonwebz

This comment has been minimized.

Copy link
Member

pattonwebz commented Nov 9, 2019

Since this one is still a bit of a work in progress I think we will need to move it over to trac and considered after WP 5.3 release.

When this repo is being archive/closed I can port this PR over to a trac ticket and create a patch file for it with the changes here.

@pattonwebz

This comment has been minimized.

Copy link
Member

pattonwebz commented Nov 11, 2019

This one is more important than I had realised before now and we should merge this and try tackle any adjustments that it needs today with priority.

Copy link
Member

pattonwebz left a comment

I am approving this as-is so it can be merged and worked on farther through a followup PR.

@pattonwebz pattonwebz merged commit fe07e33 into WordPress:master Nov 11, 2019
1 check passed
1 check passed
Travis CI - Pull Request Build Passed
Details
@karmatosed

This comment has been minimized.

Copy link
Member

karmatosed commented Nov 11, 2019

@yannickiki there have been a number of changes lately, would love to see if you feel better about these, also #994 has some adjustments too. Thanks @allancole for working on this to get it to a better space for group block.

@yannickiki

This comment has been minimized.

Copy link
Contributor

yannickiki commented Nov 11, 2019

@karmatosed @allancole I’ve tested the latest commits. Unfortunately, all the issues that I’ve mentioned above remain. The most critical is the first one (1a & 1b).

To reproduce the issue:

  • Insert a left-aligned pull quote
  • View the front-end at different screen size (from mobile to large desktop)

While it’s working for a 1280px screen (like a 13-inch macbook) it’s broken at 360px, 768px, 1980px etc.

@karmatosed

This comment has been minimized.

Copy link
Member

karmatosed commented Nov 12, 2019

Noting that @allancole made a new PR here: #998. This should now soothe the smaller screen issues.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.