Generated by 🚫 dangerJS

You can test the changes on this Pull Request by downloading it from AppCenter here with build number: 34170. IPA is available here. If you need access to this, you can ask a maintainer to add you.

You can trigger optional UI/connected tests for these changes by visiting CircleCI here.

@geriux this is looking really solid. Nice work! A few things I am seeing when testing the latest build on WPiOS:

• On phone (using full-width alignment), I think we can extend the left and right outer borders a bit more so that they extend beyond the edges of the viewport. I"m not sure if that should just be an additional 1px in negative margin or if we want to maintain the usual 12px spacing that currently exists between the selection outline and block contents. In the process, we can also lose the border-radius. (example)
• I noticed that if I add a Group block and make it full-width, then add a Columns block or other children, those children aren't taking up that available space. Is it possible to force the innerBlocks to utilize all of that available space? (example). Similarly, I also noticed that when I create a Group block, the inline appender doesn't resolve to fill the entire space, but it does if you add something as a child and re-select the parent Group.
• Are we manually adding support to specific blocks? I noticed that the Columns, Media Text, and Gallery blocks didn't have support so I am assuming that is the case.

I will be doing another round of review this weekend or on Monday, but I wanted to take it for a quick spin before shutting down for the week 😄 Awesome work thus far, can't wait to get this rolling!

@geriux this is looking really solid. Nice work! A few things I am seeing when testing the latest build on WPiOS:

Hey @iamthomasbishop Thank you for giving it a first review!

• On phone (using full-width alignment), I think we can extend the left and right outer borders a bit more so that they extend beyond the edges of the viewport. I"m not sure if that should just be an additional 1px in negative margin or if we want to maintain the usual 12px spacing that currently exists between the selection outline and block contents. In the process, we can also lose the border-radius. (example)

A new build is being made now that fixes the above issue =)

I also noticed that when I create a Group block, the inline appender doesn't resolve to fill the entire space, but it does if you add something as a child and re-select the parent Group.

The Group block appender will be fixed in the latest build. Thanks for catching that!

I noticed that if I add a Group block and make it full-width, then add a Columns block or other children, those children aren't taking up that available space. Is it possible to force the innerBlocks to utilize all of that available space?

These blocks aren't taking up the available space because it's disabled. As you mentioned, yes we are going to start manually adding support to specific blocks because some of them aren't prepared yet, like Columns, Gallery, etc.

Are we manually adding support to specific blocks? I noticed that the Columns, Media Text, and Gallery blocks didn't have support so I am assuming that is the case.

For this first iteration, we'll go with Group, Cover, and Image while we work adding support for Columns.

I will be doing another round of review this weekend or on Monday, but I wanted to take it for a quick spin before shutting down for the week 😄 Awesome work thus far, can't wait to get this rolling!

Thanks for testing! If you haven't started that second round, could you please do it with the new build? Thanks a lot!

@geriux Thanks for the quick fixes and the updated build!

A new build is being made now that fixes the above issue =)

This is a tiny thing and I should've thought of it when making my proposal, but a side-effect of the 1px negative margins is that the border now gets cut off by the safe area on phone in landscape orientation. Here is a screenshot for reference. The change looks great on portrait orientation and on tablet. 👍

The Group block appender will be fixed in the latest build. Thanks for catching that!

Has been fixed, indeed. Thank you!

These blocks aren't taking up the available space because it's disabled. As you mentioned, yes we are going to start manually adding support to specific blocks because some of them aren't prepared yet, like Columns, Gallery, etc.

Ahh, that's right. Thanks for the clarification.

Nesting spacing/sizing

The only other thing that's standing out to me at the moment is how we're spacing nested blocks (the ones that support wide/full-widths, at least). This might be a non-issue that we leave as-is, because I think themes handle this in a similar way, but I'm curious what you think.

Ok, so I was curious how a nested block with full/wide alignments would be sized and I was slightly confused by the result. More on that below. As an example, I created a full-width Cover block, then nested an image block (which supports these new alignments) and tried both full and wide alignments. In short, here's what I expected vs. what actually happens:

Wide	Full-width
Expected: block content align w/ 16px margins	Expected: block content edge-to-edge
Result	Result

...whereas on the web, wide and full-width options produce the same exact result, which looks aligned roughly to ~16px.:

The way they're aligning the wide option in that scenario makes sense, because it acts exactly as if any other wide block would on portrait/phone, but I'm not sure why a full-width block wouldn't actually be applied as full-width 🤔 . Perhaps I'm missing some context 🤷‍♂️. Either way, I'm curious to hear what you think about this -- my instinct says we should match what the most common behavior of themes is, which is that wide and full-width produces the same result (the child block's contents align to the 16px margins). Even if it is slightly confusing at first, at least it's a consistent.

Thanks for testing it again @iamthomasbishop !

This is a tiny thing and I should've thought of it when making my proposal, but a side-effect of the 1px negative margins is that the border now gets cut off by the safe area on phone in landscape orientation. Here is a screenshot for reference. The change looks great on portrait orientation and on tablet. 👍

Oops, missed that case 😅 now it should show correctly on a phone in landscape orientation with a safe area.

Nesting spacing/sizing

The only other thing that's standing out to me at the moment is how we're spacing nested blocks (the ones that support wide/full-widths, at least). This might be a non-issue that we leave as-is, because I think themes handle this in a similar way, but I'm curious what you think.

Ok, so I was curious how a nested block with full/wide alignments would be sized and I was slightly confused by the result. More on that below. As an example, I created a full-width Cover block, then nested an image block (which supports these new alignments) and tried both full and wide alignments. In short, here's what I expected vs. what actually happens:

The way they're aligning the wide option in that scenario makes sense, because it acts exactly as if any other wide block would on portrait/phone, but I'm not sure why a full-width block wouldn't actually be applied as full-width 🤔 . Perhaps I'm missing some context 🤷‍♂️. Either way, I'm curious to hear what you think about this -- my instinct says we should match what the most common behavior of themes is, which is that wide and full-width produces the same result (the child block's contents align to the 16px margins). Even if it is slightly confusing at first, at least it's a consistent.

The issue here was Cover block that had inner paddings for its inner blocks, this was causing those unexpected margins.

In the latest build, I removed this for inner blocks (within Cover) that have wide or full-width alignment option selected.

Do you prefer it like that? Or should we aim to follow what web does for mobile web?

Let me know, thank you!

Thanks for the updates @geriux!

Oops, missed that case 😅 now it should show correctly on a phone in landscape orientation with a safe area.

Looks better! I was wondering if we should also change the border position on selected media to the outside of the cell as well to achieve a similar effect, but this might not be as much of an eye-sore after we implement this proposal I have open to reduce $overlay-border-width from the current 2px to 1px instead.

The issue here was Cover block that had inner paddings for its inner blocks, this was causing those unexpected margins.
In the latest build, I removed this for inner blocks (within Cover) that have wide or full-width alignment option selected.
Do you prefer it like that? Or should we aim to follow what web does for mobile web?

Ah, that makes sense re: paddings. While my personal opinion is that I’d expect setting full-width in any case to result in the block spanning edge-to-edge of the container it’s within, it’s probably best in this case to follow the web — especially since this generally produces the same result on the front-end. So we can just roll back to the “normal” approach with inner paddings.

One thing we could do is provide a workaround in the form of a switch/toggle on block settings that explicitly removes/ignores the padding from the parent. I’m unsure if it’s worth the effort and hesitant to suggest because I have a feeling in the future Core may provide similar controls via something like Global Styles. However, if this wouldn’t require much effort, perhaps it’d be worth it — what do you think?

Here is some additional feedback:

• I noticed that if you have a full-width child inside a Cover block, and the parent (Cover) is selected, the bordering looks a little odd because the child block is above the parent in terms of z-index. Is it possible to show the selection border over the top of the child? (screenshot)
• When you start a new post, it looks like the placeholder state of the first paragraph block is being pushed out to the left, until it is selected. (screenshot)

I think that’s all I’ve got besides the issues I mentioned in the previous comment. This is looking and feeling really solid overall, nice work!

Looks better! I was wondering if we should also change the border position on selected media to the outside of the cell as well to achieve a similar effect, but this might not be as much of an eye-sore after we implement this proposal I have open to reduce $overlay-border-width from the current 2px to 1px instead.

Do you mean something like this (reducing the overlay border as mentioned in the issue)?

If it's exactly that I think it'd be best to do it in the issue you opened so this PR doesn't get too big. What do you think?

Ah, that makes sense re: paddings. While my personal opinion is that I’d expect setting full-width in any case to result in the block spanning edge-to-edge of the container it’s within, it’s probably best in this case to follow the web — especially since this generally produces the same result on the front-end. So we can just roll back to the “normal” approach with inner padding.

No problem, I added back the paddings for inner blocks within Cover.

One thing we could do is provide a workaround in the form of a switch/toggle on block settings that explicitly removes/ignores the padding from the parent. I’m unsure if it’s worth the effort and hesitant to suggest because I have a feeling in the future Core may provide similar controls via something like Global Styles. However, if this wouldn’t require much effort, perhaps it’d be worth it — what do you think?

This may be a little tricky and would definitely add more time for this task, I think it would be best to do it after we have the base (this PR) that we can then improve on. What do you think?

• I noticed that if you have a full-width child inside a Cover block, and the parent (Cover) is selected, the bordering looks a little odd because the child block is above the parent in terms of z-index. Is it possible to show the selection border over the top of the child? (screenshot)

I believe this is no longer an issue since we added back the paddings so it won't reach the edges of the parent Cover block. Let me know just in case I'm missing something.

• When you start a new post, it looks like the placeholder state of the first paragraph block is being pushed out to the left, until it is selected. (screenshot)

Nice catch! Solved.

I think that’s all I’ve got besides the issues I mentioned in the previous comment. This is looking and feeling really solid overall, nice work!

Thank you again @iamthomasbishop for the review! I updated everything and made new builds if you want to give it another quick test. 😃

Awesome stuff, thanks for the update! I think we're in good shape -- only noted one more refinement we might want to make, but not a huge issue by any means.

Do you mean something like this (reducing the overlay border as mentioned in the issue)?
If it's exactly that I think it'd be best to do it in the issue you opened so this PR doesn't get too big. What do you think?

Yea, that is what I meant -- looks solid, and if you prefer to tackle it in another PR that's totally fine.

This may be a little tricky and would definitely add more time for this task, I think it would be best to do it after we have the base (this PR) that we can then improve on. What do you think?

Sounds good to me, let's create a separate ticket so it's on our radar for potential future exploration -- I don't think we need to action on it right now.

I believe this is no longer an issue since we added back the paddings so it won't reach the edges of the parent Cover block. Let me know just in case I'm missing something.

Looks solved, thank you! 👍

Tiny thing I found as a side-effect of our decision to not force edge-to-edge on full-width child blocks. The dashed border on full-width children looks a bit awkward. I understand this is probably just because the border-style is just inherited from the "normal" full width blocks, but do you think it would make sense to bring the left/right border outward to the same key lines as the wide alignment in this case? Example (using nested Cover blocks):

Awesome stuff, thanks for the update! I think we're in good shape -- only noted one more refinement we might want to make, but not a huge issue by any means.

Yay! Thanks for testing!

Yea, that is what I meant -- looks solid, and if you prefer to tackle it in another PR that's totally fine.

Cool, will do that.

Sounds good to me, let's create a separate ticket so it's on our radar for potential future exploration -- I don't think we need to action on it right now.

👍

Tiny thing I found as a side-effect of our decision to not force edge-to-edge on full-width child blocks. The dashed border on full-width children looks a bit awkward. I understand this is probably just because the border-style is just inherited from the "normal" full width blocks, but do you think it would make sense to bring the left/right border outward to the same key lines as the wide alignment in this case? Example (using nested Cover blocks):

Nice catch! This should be solved in the latest build from this PR. Thanks again @iamthomasbishop for the design / functionality review! 🙌

Nice catch! This should be solved in the latest build

@geriux indeed solved, thank you!

I think we are in good shape from the design perspective. I think once code review is complete, it should be ready to 🚢!