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

Add Column block - 1st iteration #1430

Closed
pinarol opened this issue Oct 9, 2019 · 36 comments · Fixed by #1661
Closed

Add Column block - 1st iteration #1430

pinarol opened this issue Oct 9, 2019 · 36 comments · Fixed by #1661
Assignees
Labels
[Status] Needs Design [Type] Enhancement Improves a current area of the editor

Comments

@pinarol
Copy link
Contributor

pinarol commented Oct 9, 2019

We need to add a column block similar to web. Column block is basically Group block with columns and a placeholder state where you can pick an initial layout. Column block has column count slider in the inspector. And each column block has percentage slider.

cc @iamthomasbishop for more details

@pinarol pinarol changed the title Column block - 1st iteration Add Column block - 1st iteration Oct 9, 2019
@pinarol pinarol added the [Type] Enhancement Improves a current area of the editor label Oct 9, 2019
@jbinda
Copy link
Contributor

jbinda commented Oct 11, 2019

@iamthomasbishop

I have started to take a look how to implement this block.

First thing is I would like to deal with RangeControl here. In the meantime I will provide some initial setup and try to pull as many logic from web as I can.

My main concerns are connected about limited space to render multiple column layout. Do you have any concepts about design ?

@iamthomasbishop
Copy link
Contributor

Apologies for the delay on design, I keep getting pulled into other things so I haven't had a chance to catch up on this block 😬

In the meantime I will provide some initial setup and try to pull as many logic from web as I can.

Sounds good 👍

My main concerns are connected about limited space to render multiple column layout.

In terms of display/layout, we can think of the Columns block like a Group block with two placeholder children (two Column blocks) – and stacked by default (like the current web version). Just a very quick rough sketch:

IMG_0647

@pinarol
Copy link
Contributor Author

pinarol commented Oct 11, 2019

like the current web version

Web version is switching to horizontal layout in mobile devices for landscape mode, or more correctly: ‘if the width is big enough’. Should we put some kind of logic similar to that? We used a similar approach on media & text too, but ofc it is only limited to 2 columns.

We can also think this for the next iteration:
We should be able to use horizontal layout but let some columns overflow to the second line when it is not fitting to screen anymore, like word wrapping. @iamthomasbishop

@iamthomasbishop
Copy link
Contributor

Should we put some kind of logic similar to that?

Ideally, I think we should use a similar approach to web if possible. Something like this (video example of web):

  • 2 or fewer columns: use similar behavior to the Media & Text block,
  • 3+ columns: follow web – display all columns on 768+, then collapse to two-per-row in middle breakpoints, then collapse to a single column (stacked) on smaller than 480 like we do w/ Media & Text

I believe this would mean that most landscape phones (in between ~400 and ~600 would show two-per-line, and tablets (~700+ mostly would show all columns across. We also need to be mindful of split-screen tablet displays, as well.

@iamthomasbishop
Copy link
Contributor

iamthomasbishop commented Oct 14, 2019

@jbinda @pinarol This is a quick first pass at the Columns block:

Columns Block

Notes:

  • All of the nested hierarchy and highlighting should utilize the work we've been doing on the Group and Media/Text blocks.
  • Also, like the Group block, we don't have the toolbar icon yet – while we're trying to get this added as soon as possible, we also have the Floating Toolbar breadcrumbs to rely on.

I also still need to attack the Column blocks within this, I'll add more on that tomorrow or Wednesday.

@pinarol
Copy link
Contributor Author

pinarol commented Oct 15, 2019

@iamthomasbishop Can we keep the settings part same between mobile and web? I mean, the same control can sure have different designs on mobile and web but here we are using a different control for number of columns, while Slider is being used on web. We still want to have Slider and Stepper separately for mobile right?

We can for sure do it like this if you feel strong about it. I just want to mention that it is contradicting with our long term goal about x-platform development. The web implementation of block settings are currently working on mobile as is, we still have some missing pieces but we already did good amount of work to make it possible. As I said, if you feel strongly about this we can do that too.

@pinarol
Copy link
Contributor Author

pinarol commented Oct 15, 2019

However, if you think Stepper should be the mobile equivalent of web's Slider it is all OK then. That way it'd be still compatible with x-platform methods.

@iamthomasbishop
Copy link
Contributor

Can we keep the settings part same between mobile and web?

I don't love the Slider component for a granular numerical selection that allows a small range (1-5 in this case). A Stepper or Picker is probably better in this case, even on web.

If we stick with a Slider, can we at least add discrete "tick" marks to make it more obvious at a glance? See Discrete Slider example in Material guidelines. I think this might be the default for short-scale Sliders, looking at this example.

We can for sure do it like this if you feel strong about it. I just want to mention that it is contradicting with our long term goal about x-platform development.

Fair enough, but I think it's very important for us to avoid going cross-platform for the sake of making our lives easier at the expense of our platform-native end-users. We should always try to find common ground, but the platform best practices will almost always win out in terms of usability in the native context.

@pinarol
Copy link
Contributor Author

pinarol commented Oct 15, 2019

@iamthomasbishop It is totally ok to choose a different component from web, my request here is just making it consistent. RangeControl is part of the public @wordpress/components library, so we should make a decision about what it should render on mobile and stick to it that’s all. This is not about making things easier, we can define mobile RangeControl as a control rendering a stepper, that’s totally cool. But if we want to render different things on cases where RangeControl is used then it’ll become a problem in terms of defining the public api. 3rd party devs will also use this same component while developing 3rd party blocks.

@pinarol
Copy link
Contributor Author

pinarol commented Oct 15, 2019

ok I had a small convo with @iamthomasbishop on this and I am attaching the summary of it here:
WordPress/gutenberg#17282 (comment)

TLDR: we can use RangeControl(slider) in Column block until we have a StepperControl component ready. In the meantime we don't need to block shipping Column block.

@jbinda
Copy link
Contributor

jbinda commented Nov 12, 2019

I pin @iamthomasbishop comment referring to column block in navigation-down thread

and open PR for Column Block soon

@jbinda
Copy link
Contributor

jbinda commented Jan 9, 2020

Hi,

I would like to summarise work progress on Column Block after my absence.

Firstable - please check the below GIF presenting current state.

1. Current State

Items that I did:

  1. Add Columns and Column block
  2. Implement flow which allows to add Columns in Inserter and Column only as a child of Columns block. Changing the column numbers can be achieved with slider in Columns block settings - i's limited between 2 and 6.
  3. Solve issue with Breadcrumbs - initially I had an issue to see proper Blocks structure when nesting freshly add blocks. The reason was icon styling that goes to Breadcrumbs.
  4. Implement logic to show the column/columns placeholder and inserter when the block is empty
  5. Add InspectorControls with Columns settings and slider to adjust columns numbers

At this stage my concerns are:

  1. I deal with Appender issue which sometimes do not opens Inserter ( in debugger I see that the opening action is triggers and after that hiding actions fires immediately after). When you start to play with block ordering the Inserter starts to work again. It looks familiar because recently I do some research on AppenderButton according to this PR. Will take a a closer look on that
  2. I have noticed that when you reduce the columns number (see on GIF) it delete the column with all of the content (which is fine - the web version works in the same way). My point here is that it's quite easy to remove more columns that you want to using the slider even if the action triggers when user release slider thumb. User can bring back remove content in previous structure with undo button. However still it's easy to loose the post unsaved changes. I believe that stepper that replace slider will help on that - am I right ?

2. Horizontal Layout
I have started to implement Horizontal Layout similar to Media Text. When it will be ready I will post the GIF. Here @iamthomasbishop described some rules about the size of columns but there is also an option on web to fix the columns percentage width - how about that ?

At this moment I did:

  1. Added settings to Column block which allow to set the percentage width of the columns in the same way as on the web. By default the columns takes space evenly
  2. Use the logic from web to recalculate rest column width in columns block after change width of one of the column - in vertical position columns is always stacked at this moment

At this stage my concerns are:

  1. On web version there is alignment option - we want to attach it in this iteration as well ?

3.Other Features

  • Layout selector as a placeholder after adding Columns ( refer to this ). If I'm correct we agreed to move it to out of scope in V1 of column block
  • replace slider with stepper (refer to this )

If there is something that I missed above and it should be implemented as a Column Block part please let me know ! :)

@pinarol @iamthomasbishop what do you think ?

edit: One more note - the code in PR can still be messy and after refactor I remove draft flag

@pinarol
Copy link
Contributor Author

pinarol commented Jan 9, 2020

hey @jbinda thanks for the summary.

I have noticed that when you reduce the columns number (see on GIF) it delete the column with all of the content (which is fine - the web version works in the same way). My point here is that it's quite easy to remove more columns that you want to using the slider even if the action triggers when user release slider thumb. User can bring back remove content in previous structure with undo button. However still it's easy to loose the post unsaved changes. I believe that stepper that replace slider will help on that - am I right ?

Right, and the stepper component is merged & ready to use so we don't have any reason for using slider for column count anymore. let's switch to stepper in this 1st iteration.

  1. Horizontal Layout
    I have started to implement Horizontal Layout similar to Media Text. When it will be ready I will post the GIF. Here @iamthomasbishop described some rules about the size of columns but there is also an option on web to fix the columns percentage width - how about that ?

cc @iamthomasbishop

Our readable content width is 512 max currently, even on tablets. So even if we put the logic "display all columns on 768+" it won't be active. This means we have even less space.

BTW, when I check web, it shows some buggy layout in mobile Portrait(iPhone X):

4 columns:

Screen Shot 2020-01-09 at 16 20 29

6 columns:

Screen Shot 2020-01-09 at 16 31 27

It doesn't look like it is an option for mobile to divide 1 row into 6 columns. We need to come up with a better way.

IMHO "columns percentage width" is pretty tricky UX-wise. User has a pretty small viewport when Bottom sheet is open. Even if we let the columns wrap, user won't be able to see all columns at once and compare them with each other. It can be interesting if we try to demonstrate column ratios on a dummy view instead, for example once the user taps column ratio settings a new modal opens with showing representative empty columns, all in 1 row, and user can arrange them there instead of the real editor. Just an idea.

But if we really do want to demonstrate this live in the editor, first thing that pops into my mind is letting the columns wrap and get divided into next rows by increasing the total width shared as the column count increase, I tried to explain it as below.

For example, the screen width is 400


2 columns, ratios: %20 %80

Share 400 among rows

|---------400--------------| //Screen width

|--80--|--------320--------| //Columns


3 colums, %20 %60 %20

Share 600 among rows:

|----------400--------------| //Screen width

Columns:

|----120----|
|------------360---------|
|----120----|

3 colums, %20 %20 %60

|---120---|---120----|
|-------------360---------|


4 columns, %20 %40 %40 %20

Share 800 among rows:

|---------400--------------| //Screen width

Columns:

|----160----|
|---------320---------|
|---------320---------|
|----160----|

...
...
and so on...

But as I said since the user won't be able to see columns on the next row there will be no chance to compare.

On web version there is alignment option - we want to attach it in this iteration as well ?

I'd really want to include this one assuming it will be pretty similar to media-text. But if that turns out to be too tricky let's talk.

I hope I didn't miss anything.

@jbinda
Copy link
Contributor

jbinda commented Jan 13, 2020

Right, and the stepper component is merged & ready to use so we don't have any reason for using slider for column count anymore. let's switch to stepper in this 1st iteration.

Ok

BTW, when I check web, it shows some buggy layout in mobile Portrait(iPhone X):

I have also noticed this behaviour. Playing with that I saw that the layout breaks when you add new columns when there was fixed percentage ratio before or if you manipulate percentage ratio and exceed some value depending on how many columns you added. Further more using the logic from web on mobile I saw that updateWidth function which is responsible to recalculate width of each Column added to Columns block returns negative value in some case. Didn't check if it has the same negative-returned value behaviour on web but this might be the reason of buggy layout you mentioned. I will do that check today and back with answer.

edit:
Please check below details about issue mentioned above:

I have checked and on web version the negative column widths value is also appears. I checked the values stored in attributes in edit.js under Column block:

To replicate it do as follow:

  1. Add Columns block with default layout
  2. Add third column
  3. Stretch one of the column to very high width ratio (e.g 95%) - it this case the layout should be still fine
    4 Try to add new column

After doing that you case noticed layout breaks and see negative values in one or two Column Attribute. For larger screens layout breaks might not be observable - but negative value should be noticed.

It doesn't look like it is an option for mobile to divide 1 row into 6 columns. We need to come up with a better way.

On vertical layout we agree to have the columns stacked and that's fine on mobile.

When rotated ideally it IMO should show the exact layout with ratio set on columns. The issue here is that we can not even display all 6 columns without breaking the layout (not saying about additionally set up percentage width). See image below:

Here is the GIF presenting 3 columns in row. In my opinion it's the limit - adding more columns to the layout make it very hard to use. As Pinar pointed on smaller screen it also can start to breaks. Also the navigation to change column order it's not intuitive as @iamthomasbishop mentioned before. We plan to change direction of the arrows ?

But if we really do want to demonstrate this live in the editor, first thing that pops into my mind is letting the columns wrap and get divided into next rows by increasing the total width shared as the column count increase, I tried to explain it as below.

I like the solution both solutions - the dummy layout and wrapping the columns. However both has props and cons.

For the dummy layout the case is like you mentioned user will loose possibility to compare the exact output. Also when I thought about the Editor it clearly shows what will be the output after save the post. Going this way I'm afraid we loose this visual experience a little bit. The benefit is that rendering the columns block will be very simple (we have all what we need at this moment - see provided GIFS before)

Wrapping the columns allows to stack columns in row depending on order in layout. I just wonder if we do not "waste" too many space on sides putting this in that way. The benefit is that user see how each of the columns looks like. I also think that if we wrap something we should also center so according to given example below:

  1. 3 columns, %20 %20 %60
|----------400---------------| //Screen width
|---120---|---120----|
|-------------360---------|

it becomes more like this:

  1. 3 columns, %20 %20 %60
|-----------   400  ------------| //Screen width
|  |--- 120---|   |--- 120---|  |
|  |-------------360---------|  |

@pinarol Am I correct that this scenario assume 200px initial width on each added column ?

Not sure if we have another option without going for compromise like in discussed solutions.

@iamthomasbishop do you have any thoughts on that ?

@jbinda
Copy link
Contributor

jbinda commented Jan 20, 2020

I've made a progress on Columns distribution according to discussion on Slack and above.

Please check below GIFs:

iPad layout

iPhone X layout

The Columns layout depends on the block container size so it allows nested Columns to be render without break the layout ( in some point the layout of inner Columns block will be stacked ).

Because we have the maximum available width hardcode to 580px even on iPad we show layout with 2 columns but the implemented logic is ready to show all 6 columns according to this comment if we extend this limit.

UX notes:
@iamthomasbishop are you able to answer below questions ?

  1. Does Columns button in Toolbar should have any action onClick ? Or it's just to inform user you have Columns block selected ?
  2. I have added verticalAlignment toolbar to set the alignment of the columns (I followed the Media Text which has the same option there). Is it proper place to put alignment settings there ?
  3. You can noticed that Appender button when you have Columns block selected has dashed style border. It's because it use 'navigation-down' styling logic made for InnerBlock. Is it ok or we need to get rid of it (be aware that it can be tricky according to mentioned logic ) ?
  4. Also the margin in the middle between column is bigger than side margins (again it's because I split available space for columns and then the bordering logic is applied in each column container. In the result we have multiplication of margins in the middle) How about that ? From implementation point of view it'a another check in bordering logic which check the position of the column in the layout and decrease both margins or left/right margin.
  5. How about the toolbar arrows in Column blocks. Do we want to change the orientation from Up/Down to Left/Right ?

Features Left

  1. Vertical Alignment for single columns. I did option to align on Columns block level which set up alignment for all Column. We can easily add control to modify it's attribute but show that on layout is more complicated. We can discuss in more detail in conversation in PR or if it's not a priority now do it in V2 of Column Block

Issues

  1. It's hard to test nested columns on iOS because of this and this issue. On Android it is ok
  2. Inserter issue. I've mentioned that there is an issue with Inserter. It occurs when the Column index inside the Columns block is equal to count of Inner Block inside that Column. The issue with the Inserter occurs before and I'm not sure if it's not something deeper that my code reveal. I feel like it can be something with showInsertion point. For sure I will take a closer look on that when the PR will be Open
  3. "Jumping" layout on Android. I think slight adjustment in implemented logic should sort it out

Side notes:

  1. I have also notice that putting Media Text block in nested Columns do not cause Media Text show in stacked layout. It's because it base on screen width instead on it's parent container width. Do we want to change this behaviour ?

@pinarol
Copy link
Contributor Author

pinarol commented Jan 20, 2020

I have also notice that putting Media Text block in nested Columns do not cause Media Text show in stacked layout. It's because it base on screen width instead on it's parent container width. Do we want to change this behaviour ?

Yes I think so, otherwise it'll act weird in nested blocks. I believe this can also be reproduced in Group block with enough nesting. @lukewalczak this can be a next task for you, would you mind taking a look into this? (+ opening an issue if you think it needs solving ) Thanks in advance! ps: you might need this PR to be able to test on iOS.

@pinarol
Copy link
Contributor Author

pinarol commented Jan 20, 2020

Vertical Alignment for single columns. I did option to align on Columns block level which set up alignment for all Column. We can easily add control to modify it's attribute but show that on layout is more complicated. We can discuss in more detail in conversation in PR or if it's not a priority now do it in V2 of Column Block

Vertical Alignment is only demonstrable when items are side by side, so we can't show it with only 1 column, no? Please correct me If I am missing something. Just like media-text, we are only able to demonstrate the vertical alignment change when media and the text is horizontally stacked.

@jbinda
Copy link
Contributor

jbinda commented Jan 21, 2020

Yes I think so, otherwise it'll act weird in nested blocks. I believe this can also be reproduced in Group block with enough nesting

I'm not so sure about that. Implement bordering logic of nested blocks in group eliminate margins multiplication so even deeply nested element should have the same size.

@lukewalczak this can be a next task for you, would you mind taking a look into this?

If the logic that I have implemented in Columns block about checking the available width is ok I can also replicate it to MediaText meanwhile we are dealing with Columns block. It's up to you if you want me to take care of it as well without Luke's engagement

ps: you might need this PR to be able to test on iOS.

Ok, thanks ! I will give it a shot

Vertical Alignment is only demonstrable when items are side by side, so we can't show it with only 1 column, no? Please correct me If I am missing something. Just like media-text, we are only able to demonstrate the vertical alignment change when media and the text is horizontally stacked.

That's true - user won't be able to see changes in vertical(stacked) layout but even then I think he should be able to change the alignment. In MediaText the VerticalAlignment Controls are visible all the time (see below screen)

My comment about verticalAlignment refers also to web behaviour. On web you can set up column vertical alignment on Columns block level or individually for each Column. See below GIF:

This feature on web also seems to be a little bit buggy in my opinion (it change both vertical and horizontal position and also change the placeholder width) I wonder if we want to implement feature to individually change the Column verticalAlignment

@pinarol
Copy link
Contributor Author

pinarol commented Jan 21, 2020

That's true - user won't be able to see changes in vertical(stacked) layout but even then I think he should be able to change the alignment

Right, they should be able to change the alignment even if they don't see it effecting the editor in some cases. When you said "Vertical Alignment for single columns" I thought you were referring to a situation where we can display only 1 column at a row, that's why I thought it was not demonstrable. Anyway, so yes, every individual column should also have its own verticalAlignment option. But I agree that web behavior looks buggy because it is changing horizontal alignment as well. And when I check the preview of the page there's no change in the horizontal alignment as expected.

If the logic that I have implemented in Columns block about checking the available width is ok I can also replicate it to MediaText meanwhile we are dealing with Columns block. It's up to you if you want me to take care of it as well without Luke's engagement

Both is fine, I thought @lukewalczak needs a new task while you still have things to do on Column. I'd just want to have that fix on a separate PR and rather quicker so we won't have blockers to ship Group anymore.

@lukewalczak
Copy link
Contributor

That's cool for me, will handle it

@jbinda
Copy link
Contributor

jbinda commented Jan 21, 2020

I'd just want to have that fix on a separate PR and rather quicker so we won't have blockers to ship Group anymore.

Ok, I have already explain Luke what should be done there and as he wrote he we will manage it

Anyway, so yes, every individual column should also have its own verticalAlignment option.

I already did some investigation on it. That's the case that it is easy to add ability to change Column block attributes but it's more difficult to render it properly because of the complexity level that InnerBlock bring is. Like I wrote before I would like to move discussion about it to PR conversation to point some code snippets

@iamthomasbishop
Copy link
Contributor

Hey @jbinda, sorry for the delays in getting back to this one. I'll answer your earlier questions and try to get caught up on the rest of the context.

Does Columns button in Toolbar should have any action onClick ? Or it's just to inform user you have Columns block selected ?

Ideally, all block types should have the selected-block icon, but it is nice that we at least have it here. Tapping on the icon-button wouldn't do anything necessarily, but it would be interesting if we could in the future show a tooltip or re-highlight the block when it's tapped to have some feedback.

I have added verticalAlignment toolbar to set the alignment of the columns (I followed the Media Text which has the same option there). Is it proper place to put alignment settings there ?

I was looking at the Columns block on web (testing on a small screen in Safari's responsive mode), and their verticalAlignment control is super confusing — when I tap on one of the controls, it actually changes horizontal alignment. And really, it's confusing and clunky on all screen sizes, some blocks do align properly and some don't based on my quick testing.

With that said, if we can get our alignments to work properly, I would expect the verticalAlignment controls to behave similarly to Media & Text.

You can noticed that Appender button when you have Columns block selected has dashed style border. It's because it use 'navigation-down' styling logic made for InnerBlock. Is it ok or we need to get rid of it (be aware that it can be tricky according to mentioned logic ) ?

The bordering doesn't look "bad" necessarily, but I think when the appender (solid border with (+) icon) sits inside a dashed border (child indication), we could remove the solid border from the appender — at least visually/CSS, so there aren't double borders. Does that answer your question?

Also the margin in the middle between column is bigger than side margins (again it's because I split available space for columns and then the bordering logic is applied in each column container. In the result we have multiplication of margins in the middle) How about that ? From implementation point of view it'a another check in bordering logic which check the position of the column in the layout and decrease both margins or left/right margin.

Obviously it'd be ideal if the gutters were the same as the side margins, so if we can shrink the gutters so that there is 16px gutters and 16px margins (on sides) that would be great. But it's probably something we could iterate on. I think it's the same concern on web, so if we can improve upon this, bonus points.

How about the toolbar arrows in Column blocks. Do we want to change the orientation from Up/Down to Left/Right ?

This is a tricky one. I do think if we are stacking on portrait phone we are safe (because visually you can only move up or down), but on landscape we might want to change to left/right arrows.

Side note: it'd be amazing if in the long-term we could remove arrows in favor of drag & drop re-ordering but that is likely going to be a big project in itself 😄

Vertical Alignment for single columns

I think alignment could get confusing on single-column layout (example: portrait phone where we're stacking columns). The controls could get easily confused w/ "re-order".

@jbinda
Copy link
Contributor

jbinda commented Jan 22, 2020

@iamthomasbishop thanks for answers and don't bother about late response - it wasn't blocker for me.

Some of the question is solved and some needs more discussion/confirmation. Please check my response below.

Ideally, all block types should have the selected-block icon, but it is nice that we at least have it here. Tapping on the icon-button wouldn't do anything necessarily, but it would be interesting if we could in the future show a tooltip or re-highlight the block when it's tapped to have some feedback.

Ok so I have the icon in Columns block but how about show icon for single Column that is a child of Columns block ? According to your answer we should probably add this icon as well, right ?

I was looking at the Columns block on web (testing on a small screen in Safari's responsive mode), and their verticalAlignment control is super confusing — when I tap on one of the controls, it actually changes horizontal alignment. And really, it's confusing and clunky on all screen sizes, some blocks do align properly and some don't based on my quick testing.

Not only the alignment but also setting the column widths (described above). Probably I'm going to open PR in gutenberg repo on that.

With that said, if we can get our alignments to work properly, I would expect the verticalAlignment controls to behave similarly to Media & Text.

This is the way that verticalAlignment is works in Columns as well

The bordering doesn't look "bad" necessarily, but I think when the appender (solid border with (+) icon) sits inside a dashed border (child indication), we could remove the solid border from the appender — at least visually/CSS, so there aren't double borders. Does that answer your question?

Yes I will try to go that way.

Obviously it'd be ideal if the gutters were the same as the side margins, so if we can shrink the gutters so that there is 16px gutters and 16px margins (on sides) that would be great. But it's probably something we could iterate on. I think it's the same concern on web, so if we can improve upon this, bonus points.

I will keep that in mind and back to this during future refactor when all other feature in Columns starts to work as expected

This is a tricky one. I do think if we are stacking on portrait phone we are safe (because visually you can only move up or down), but on landscape we might want to change to left/right arrows.

From the implementation point of view we only need to find a way to pass the information about direction of an arrow to Block Toolbar from Column block. We have access to all info to distinguish what. Keep in mind that even in landscape we can have Column stacked when we do some nesting.

Maybe we can iterate on it in V2 of Columns Block ?

I think alignment could get confusing on single-column layout (example: portrait phone where we're stacking columns). The controls could get easily confused w/ "re-order".

Yes. Not only in portrait phone layout. Even in landscape in some case user won't be able to see effect after changing verticalAlignment attribute (e.g. when all Column has the same height). The same case is in MediaText - the alignment option is still available in stacked layout even thus we can not observe the effect

According to current MediaText behaviour I think we should allow changing the vertical alignment should be possible even we can't see effect on mobile. What do you think ?

@iamthomasbishop
Copy link
Contributor

iamthomasbishop commented Jan 22, 2020

Ok so I have the icon in Columns block but how about show icon for single Column that is a child of Columns block ? According to your answer we should probably add this icon as well, right ?

I think so. Is there a component that we can add to or create to supply a re-usable "selected block icon" spot on the toolbar? If we can do it across all blocks, that'd be ideal. But if not, I still think adding icons where possible is useful.

Not only the alignment but also setting the column widths (described above). Probably I'm going to open PR in gutenberg repo on that.

This is the way that verticalAlignment is works in Columns as well

Yes I will try to go that way.

I will keep that in mind and back to this during future refactor when all other feature in Columns starts to work as expected

👍 to the last 4 items above.

From the implementation point of view we only need to find a way to pass the information about direction of an arrow to Block Toolbar from Column block. We have access to all info to distinguish what. Keep in mind that even in landscape we can have Column stacked when we do some nesting.

Maybe we can iterate on it in V2 of Columns Block ?

Sounds good to me, ship and iterate 😄

According to current MediaText behaviour I think we should allow changing the vertical alignment should be possible even we can't see effect on mobile. What do you think ?

Not ideal, but I think it's something we can address during future iterations.

@iamthomasbishop
Copy link
Contributor

iamthomasbishop commented Jan 22, 2020

Edited my responses above. Oops, I hit enter too soon. I'm finishing up my responses on the comment above 😄

@jbinda
Copy link
Contributor

jbinda commented Jan 23, 2020

@iamthomasbishop

I think so. Is there a component that we can add to or create to supply a re-usable "selected block icon" spot on the toolbar? If we can do it across all blocks, that'd be ideal. But if not, I still think adding icons where possible is useful.

At this moment to render this Icon we render ToolbarButton wrapped in Toolbar and BlockControls. So definitely we can extract the logic to simplify it's use according to add Icon to rest of the components.

Further more there is still AdvancedInspectorControls ticket (which should adds extra Additional CSS field to each block setting) waiting to be implemented. I think we can tackle the Icon component you mentioned then because I believe the structure of both will be very similar

For the rest things I will go according to above :)

Thanks !

@jbinda
Copy link
Contributor

jbinda commented Jan 23, 2020

@pinarol @iamthomasbishop

I think I handle the verticalAlignment in Columns and Column block. Please take a look:

Is it works as you expected ?

@pinarol
Copy link
Contributor Author

pinarol commented Jan 23, 2020

It looks OK to me. The focused state has FloatingToolbar which takes some space so that's why the change is not vey obvious but it'd be more obvious if the other column had longer content. BTW we started working the ideal version of FloatingToolbar lately. 🤞

@jbinda
Copy link
Contributor

jbinda commented Jan 23, 2020

That's true - the feeling about alignment effect depends on the Column contents and placement to each other in Columns block.

However according to initial concept and discussion I think it looks nice.

@iamthomasbishop
Copy link
Contributor

I agree the alignment controls work pretty much as I'd expect. 👍

@jbinda
Copy link
Contributor

jbinda commented Jan 28, 2020

@iamthomasbishop

I removed the border from AppenderButton it looks like below (please notice that the style of the icon is broken - according to the issue on Slack I mentioned)

The approach we discuss to remove the solid Appender border do not work well on Dark mode because the button gets dark-gray background color. Also there is some padding which cause that button in light mode seems to be much bigger that it really is.

I think if we want to change the look that I have posted originally we might try the solution with remove dashed border around the button. However like I mentioned before it can be a tricky to implement because we need to modify the logic we prepare during navigation-down PR

@iamthomasbishop
Copy link
Contributor

@jbinda Bordering looks nice and clean on light mode, although I agree w/ your note about the "extra" visual padding 😄 Can we remove the dark gray fill on the button in dark mode? We could also remove the extra padding so that it's not quite as large and the button tap area would take up the whole "dashed" area. Does that make sense?

@jbinda
Copy link
Contributor

jbinda commented Jan 31, 2020

@iamthomasbishop

Thanks for answering on that. Also apologise for late response because I had been dealing with the fixes for Group block.

According to your request I think we have the same concerns here as I posted in this comment

However in this case the simplest solution that I think might work pretty well is just make the dashed border transparent to visually keep only the Appender. Then we do not changing the margins (and touch the logic of border styling of InnerBlocks).

Also stretching the placeholder might be necessary to align the content but I think it can be done from Column block without changing the InnerBlock logic as well

@iamthomasbishop
Copy link
Contributor

However in this case the simplest solution that I think might work pretty well is just make the dashed border transparent to visually keep only the Appender. Then we do not changing the margins (and touch the logic of border styling of InnerBlocks).

Ok, let's try that. When you make the change, can you drop screenshots to see the diff?

@jbinda
Copy link
Contributor

jbinda commented Jan 31, 2020

Ok, let's try that. When you make the change, can you drop screenshots to see the diff?

Of course, I will post you

@jbinda
Copy link
Contributor

jbinda commented Feb 3, 2020

@iamthomasbishop

Please check below screen with adjusted AppenderButton style inside Column block. I think it looks nice. Do you ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Status] Needs Design [Type] Enhancement Improves a current area of the editor
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants