Skip to content

Conversation

@enejb
Copy link
Contributor

@enejb enejb commented May 17, 2021

This PR adds the availability layer for the layout grid block.

Related PRs:
Gutenberg: WordPress/gutenberg#31777 WordPress/gutenberg#32495
Block Experiments: Automattic/block-experiments#187 (merged), Automattic/block-experiments#206
Android: wordpress-mobile/WordPress-Android#14667
iOS: wordpress-mobile/WordPress-iOS#16543

To test:
Case 1:

  • Does the block show up for the .com site (simple and atomic). It should.

Case 2:

  • Does the block show up for a jetpack site or a .org site. It should NOT for now.

Note that this PR will require another rebase after the Gutenberg and the block-experiments PRs are merged.

PR submission checklist:

  • I have considered adding unit tests where possible.
  • I have considered if this change warrants user-facing release notes more info and have added them to RELEASE-NOTES.txt if necessary.

@peril-wordpress-mobile
Copy link

peril-wordpress-mobile bot commented May 20, 2021

Wanna run full suite of Android and iOS UI tests? Click here and 'Approve' CI job!

Copy link

@antonis antonis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work putting this together @enejb 👍
I tested this with the Android PR with composite build running metro and the app works as expected 🎉
The code changes LGTM!

Copy link
Contributor

@mzorz mzorz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Echoing @antonis here, also tested this one and works alright 👍
Also as discussed elsewhere code LGTM, approving but leaving merge for you to do later once conflicts are fixed and the other PRs are all in order to be merged 👍
🚢

Copy link
Contributor

@chipsnyder chipsnyder left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested this with the iOS Branch so I'll consolidate my Questions here.

cc @iamthomasbishop Because some of these are design questions.

Margins Seem Off

I noticed the margins extend beyond the sides of the device.

Portrait Landscape

Here's an example of Columns to compare:

Do we want to show the align options?

The only alignment option is Full width. Is this a toolbar that we want to continue to show when the user can't make a selection?

Vertical Alignment Doesn't follow the web-behavior.

I noticed that the vertical alignment doesn't follow the web-behavior. On web, it works the same as our column blocks do:

  • Columns - Changing alignment on Columns - TC009
  • Columns - Changing alignment on individual Column - TC010

Default columns

Columns will show 3 columns before it starts to wrap. Is this a desired difference?

Selecting the Block inserter

Selecting the block inserter when opening a column is selected provides an empty inserter menu.

Inserting a layout grid with in a grid column makes the column none functional

If I insert a layout grid in a grid I can't interact with those nested items.


@enejb I'm going to pause my functional testing here because it seems like there is still more work to be done. Were these known issues and the goal for this PR was to have to just have the block appear? If so I think we should have the capability off by default.

@chipsnyder
Copy link
Contributor

Retesting some of these after chatting with @enejb and I'm not getting consistent results. I'll retest more tomorrow.

@chipsnyder
Copy link
Contributor

I started to retest some of this. I didn't finish but I wanted to provide notes since I'll be AFK.

Inserting a layout grid with in a grid column makes the column none functional
This still happens for me.

Selecting the Block inserter
This also still happens. Steps:

  1. Add layout grid
  2. Select a column
  3. Press the Blue inserter button in the toolbar

Vertical Alignment Doesn't follow the web-behavior.
This seems to be a bug on the Web as well. The columns can have a different alignment but the parent is set to top when they are different. This caused some confusion for me. We should make sure to open an issue if one doesn't exist.

Do we want to show the align options?
I don't think this is a blocker but it caused a lot of confusion for me. @iamthomasbishop @enejb I wonder if we can do a second iteration and either add a "none" option or treat it like a button similar to the formatting options WDYT?

Margins Seem Off
This was a miss understanding on my part. Also not a blocker but I wonder if there are some other style hints we can provide? Mentioning because it was not something that was intuitive for me but I'm not sure yet what other options exist.

@iamthomasbishop
Copy link
Contributor

@chipsnyder

Do we want to show the align options?

I've mentioned this concern a few times in the past, and it's definitely something I would like to change. It'd be useful on a bunch of sheets (block alignment, text alignment, vertical alignment, etc).

Selecting the block inserter when opening a column is selected provides an empty inserter menu.

Let's disable the inserter button in this case.

Margins Seem Off

We've been discussing this a bit because it also threw me off (esp the fact that Layout Grid starts in full-width alignment, whereas Columns starts with "natural"/no alignment). There are some wider issues going on, so I think it'd be worth getting some attention on that (separately, if need be).

Inserting a layout grid with in a grid column makes the column none functional

Vertical Alignment Doesn't follow the web-behavior.

I'm not really sure what you're referring to with these two things, I'll have to test again next week.

@chipsnyder
Copy link
Contributor

There are some wider issues going on, so I think it'd be worth getting some attention on that (separately, if need be).

We can revisit this later in a more holistic pattern.


A few more reports I noticed:

Columns have a Delete Block Option that does nothing

Screen.Recording.2021-06-01.at.3.34.58.PM.mov
  1. Add a Layout grid-block
  2. Select a Column
  3. Select ...
  4. Select Remove Block
  5. Notice that the Block removed notice is presented but the remove block option remains

Can't interact with a grid with nested grids

Screen.Recording.2021-06-01.at.3.40.59.PM.mov
  1. Add a Layout grid-block
  2. Select a Column
  3. Insert a Layout-grid in the column
  4. Notice that you can't select the nested layout grid items.

Duplicate/Cut/Copy does nothing

Screen.Recording.2021-06-01.at.3.49.03.PM.mov
  1. Add a Layout grid-block
  2. Select a Column
  3. Add some content
  4. Select ...
  5. Select Duplicate, Cut, or Copy
  6. Notice that nothing happens

@enejb
Copy link
Contributor Author

enejb commented Jun 3, 2021

I created a bunch of issues that I think we should fix before we merge this.
#3581
#3582
#3583

as well as #3580
Which I think we should tackle outside of this project.

Vertical alignment.

I think this issue is working as expected since it works the same as web side of columns and the layout grid. From my testing.

But it suffered from similar issues as the horizontal alignment where the non or mixed state is not very clear.
Maybe we can address this issue at the same time as the horizontal alignment.

Margins are off.

This part is working as expected for now. I do agree that the expectations are a bit strange but since it works the same way on the web (Full Alignment is the default). I inclined to leave things as they are for now.

@enejb enejb force-pushed the add/experiamental-layout-grid-availability branch from ba62082 to d21517f Compare June 6, 2021 17:08
@enejb
Copy link
Contributor Author

enejb commented Jun 7, 2021

@chipsnyder and @mkevins
I think this PR is ready for another review. 🥳

I addressed all the issues. Expect the alignment ones. Since they are not layout grid specific.
And this Pr is already pretty big.

Here is what the new flow looks like.

Screen.Recording.2021-06-07.at.12.19.44.PM.mp4

@chipsnyder
Copy link
Contributor

@chipsnyder and @mkevins
I think this PR is ready for another review. 🥳

Awesome. I'll take a look at this today.

Copy link
Contributor

@chipsnyder chipsnyder left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for making those changes @enejb this is working great for me now 🎊

@chipsnyder chipsnyder added [Status] Needs Design Review Needs design review or sign-off before shipping and removed [Status] Needs Code Review labels Jun 15, 2021
@enejb enejb force-pushed the add/experiamental-layout-grid-availability branch from 17f02a3 to 9148a17 Compare June 17, 2021 19:27
@enejb enejb force-pushed the add/experiamental-layout-grid-availability branch from 2bea17c to edaefd0 Compare June 21, 2021 21:22
@enejb enejb merged commit 72b5b21 into develop Jun 21, 2021
@enejb enejb deleted the add/experiamental-layout-grid-availability branch June 21, 2021 22:02
@enejb enejb mentioned this pull request Jun 21, 2021
2 tasks
@geriux geriux mentioned this pull request Jun 25, 2021
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

[Status] Needs Design Review Needs design review or sign-off before shipping

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants