-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Block Library: Avoid column width auto-adjustment when sibling width changes #19515
Conversation
You can test this live at: http://gutenberg.run/19515 |
My initial test proved this to be functional and harmless. I set a Columns block with 3 columns; 30%, 50%, and 60%. The columns stayed aligned with the content width, and when changing it to wide-width, they still worked a flowed really well. Editor screenshots: and As a user, while I was completely out of the 100% range, the columns still looked fairly correct to me. I was quite happy with the results. I still don't like jumping around from column to column to adjust the sizing, but this end result worked wonderfully. |
Yeah, in testing the behavior here, I made a remark at #19397 (comment) to a particular block selection behavior which was changed there. I'd not go so far as to say it's a regression, but especially when trying to resize columns immediately after inserting the Columns block, it can be a pain to try to select them. |
@talldan If you have a free moment, would you be able to give this one a pass at code review? Pinging since you had given some eyes to a previous, related pull request which sought to solve the same problem in a different way (#16822 (comment)). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code looks good, and in terms of usability it's now much easier to set manual width 👍
To this last point, this can potentially cause some confusion of its own, since when adjusting the width of a column, you may still observe that the other columns grow or shrink automatically, but only because they are not (yet) assigned with an explicit width, and thus adjust themselves flexibly.
@aduth I agree with that, that is potentially confusing.
What I imagine should happen is that any column with an unset width takes up the remaining space (sharing it equally with any other unset columns). That way if I had four columns, made the first three 25%, the last one would automatically be 25%.
What seems to be happening at the moment is that a column with an unset width by default has a flex-basis: calc( 50% - 16px )
, so it's like its set to just under 50% by default.
I still think this PR should be merged as it's a nice improvement.
@mapk Do you have any thoughts on this and whether it's a problem worth worrying over? I can see arguments both ways actually, where it's either potentially confusing in how some of the columns are still flexibly arranged, vs. convenient in cases where I might want those remaining columns to be flexibly arranged.
This could work, and in fact is basically the same as what we have today, except that I assume we'd only want it to redistribute the width on the first time the block changes from a state of unset widths to explicit widths, and avoid further redistribution from that point forward. |
I am testing the playground (very neat btw) and had a difficult time selecting a column. I had to change the tool to select and then it worked. I added 3 columns. Click the first column added 25%. Clicked the 3rd column added 25% here as well. Clicking the middle column I kinda expected to see the percentage width numbers of 50%, but no numbers are listed. It would be great to have an edit icon one can click to have the various visual initial column options show up again, so one can click a new column layout. For instance this time selecting a bigger column on the right and two smaller on the left. Btw I also look forward to being able to drag the sides of a column to resize it. I could of course mention a lot of other column features as well that do not belong in this issue. Such as gutter between columns, background color and on and on...:) |
I think there might have been some unrelated issues at the time the branch was created. It might be better in master? I know it's not a great experience in general. That said, the changes here don't really (and aren't intending to) have an impact for better or worse on the column selection experience, but rather the width adjustment behavior.
I think this reinforces what @talldan and myself expressed as possible concern, where perhaps we should force all columns to have a width, at least the first time the blocks change from "fully flexible" to any of the blocks having a specific width. |
Hi @aduth I ran a few tests via your tester at http://gutenberg.run/19515 and I can see the benefits of the non-100% approach. If you set percents in all the boxes then you can achieve a variety of effects. 10% | 10% | 10% gives you a total width of 30% with a 70% right blank area. 50% | 50% | 50% gives you the equivalent of 100% in equal thirds. There are a few anomalies though from a U.X. perspective:
Although the dynamic total %width approach looks promising I'm not sure how well it will be received by the average end-user or average designer who is used to working to a pre-set columns proportion and a mental reference point of 100% total width. The lack of visibility of the current widths and the conceptual floating width % leaves the end user / designer uncertain as to what the widths are unless they set all of them manually. If certainty can only be achieved by setting ALL of the boxes manually, then the simplest U.I. is one that re-inforces that convention...use the default or set them all. As per [my previous suggestion](#17918 (comment) and #17918 (comment).) I would humbly suggest we follow historic convention and stick with:
|
Sorry for a late response here. I messed around with this again and agree with @talldan's comment here:
While setting the widths currently, the undetermined width on the columns seems to be very out of relation to the other widths. When setting each column width, the last column was abnormally wide. When "reseting" the last column's width, I expected it to remain at the left over percentage amount, but instead it went wider than expected again. |
Has anyone run this past the accessibility team? The new approach proposed here does not make visible the actual default column widths or the notional total width% in this new floating width approach. How is a user / designer with a screen reader going to adjust column widths? |
Took this branch for a quick spin and inserted a 3 column block. Here's the behavior I noticed:
I am possibly in the minority, but I wanted to explicitly voice support for this approach. It respects explicitly input numbers, and makes it more predictable how columns are sized, when done so manually. As part of a recent self imposed empathy challenge I tried creating some block patterns, and the following is a group containing two column blocks in succession: The first column block has 2 columns, the next one has 3, and it was exceptionally painful to get the 33% value I explicitly input in the 3rd columns of both, to stick. As far as I can tell, there's only one downside to this PR. Unless all three column widths are somehow defined, only those actually defined are correctly sized. Here, the first column is set to 80% width, and the 2 remaining columns are unset: But obviously the sizes aren't correct. Is this a limitation of the flex based nature of the columns block, or can |
Hi @timhibberd, can you elaborate on this?
If a column does not have a set width, barring what's possibly a bug, an empty text input field should be correct. As for actualy setting these values using a screen-reader, this should be no different from what the behavior is in master. |
@jasmussen - my comment about accessibility is related to the combination of no display of the actual column widths coupled with a floating concept of total width as a percentage being some number other than 100%. I have eyesight and I'm not sure what the widths are. I'm not sure how a screen-reader would figure out what I can't. Please have a look at my post that preceded yours. This lists all the issues with the current implementation and my strong concern that this approach, though clever, will not be understood by the end-users. This is a significant departure from conventional UI for a Columns editor with no end-user focus testing and no accessibility team input. I proposed a conventional approach that addresses all the outstanding issues using conventional tactics. No one has commented on it pro / con though. Happy for you to be the first :-) |
Andrew is skilled in best accessibility practices, I did not get a sense that any concessions were suggested, nor should any be made. I added an "Needs Accessibility Feedback" label to give this visibility and sanity check this further. I think we may be talking past each other, though: what I'm suggesting is that the the clever approach — automatically calculate and explicitly set column-widths — is possibly a source of confusion, as it overrides user-set values. And instead, that a simpler approach, behaving like a standard HTML table, would probably be preferable. That is — if a column has an unset value, it literally does not have a width that we can surface visually, because that width is calculated by the browser. |
@jasmussen - I agree with your point that the browser would choose the width so displaying a visible width is potentially inaccurate. The point I was trying to make is that this implementation has an arbitrary and unstated concept of what the total width is. This will weird out end-users who think in terms of 100% (except footballers who believe 110% is possible) and designers who want to specify a verifiable percentage. I have dealt with some very pedantic clients who require pixel-perfect. renderings. I also pointed out that at various settings I have no idea what the widths of the unspecified columns are. Set the first column to 100% and you tell me what the widths are of the other two. You and I may not care as long as it looks good, but I know some clients who will want to know exactly what the widths are. This may also become more of an issue when we allow the inter-column margins to be adjusted. So my suggestion was...make the UI re-inforce that if you override the default you set all three columns or if auto-set then once you set the first column the other two columns split the difference. |
I may be giving too much creedence to the table block. It truly can get you into trouble if you really want to watch the world burn: The equivalent here is to set 80% in all three columns, which obviously is more than a hundred. What happens then? In the table example, this breaks it. It seems like we could let the columns block be 240% wide, causing a horizontal scrollbar. It might not be ideal, but it would be what the user asked for. It becomes a bit of a question of how much handholding to do. Take the paragraph block, you can make this atrocity happen: The text is barely legible, no-one should do this. While we don't prevent the user, we do show a warning note about the contrast. Would a similar warning note, added to a Columns block with more than 100% total width as defined by the user, mitigate the trouble users can get themselves into? |
0b2df9f
to
92b8fd4
Compare
Last-ditch effort, though see note below about potential blocker:
Can be tested at: http://gutenberg.run/19515 Important Note: I don't think the front-end styles of columns are currently correct to support the use-cases we're targeting in allowing mix of flexible and fixed-width columns. Notably, these styles: gutenberg/packages/block-library/src/columns/style.scss Lines 36 to 37 in fab9e02
This has strange results when trying something like I might expect something like You'll have to forgive my brevity, as it is the end of a long day. I'll plan to revisit when I am of a fresher state of mind. |
Nice "Down to the Wire" save :-) I am good with change 1 and change 2. By forcing the user to switch from auto to manual it reinforces that the user does not need to do anything if they are content with the default. Most of my clients will be content with the block level default choices and won't feel the need to press the "manual button". I also like that change 1 allows the default column units to now be displayed in the input box when in manual mode. Minor Adjustment: You might consider changing "Manual width" to "Manually adjust width" to be consistent in verbiage with the other radio button option. NOTE: Your mixed mode seemed to work fine. Even the 20 | auto | 20 at least on the latest Firefox. Minor Adjustment: If you could make the width input box wider so the default two decimal place numbers (e.g. 33.33) do not look squashed. And at present double-click selects only the integral part rather than the whole number including the decimal part. You probably already had that on your to-do list since you got this revision done lightning fast (kudos for the quick pivot :-) Minor Adjustment: If you set the columns to 25 | 50 | 25 and then increment the 3rd column: No message pops up for 25.1 through 25.4 but an error message will popup at 25.5 Side-Note: It would be great to get rid of the double vertical scroll bar as you see in the screenshots if possible. |
This is impressive work. Thank you all for staying on this. Today is the day, the last day to get a fix in before the beta period. But it's important to remember that work won't stop today. The only constant is change! To review, the problem is that in the shipping product, users can't set the width of more than 1 column. As soon as they set a single column, the other columns are auto-distributed the remaining space. This is what I see in the branch: This is a good suggestion for adding clarity. Other solutions could be to offer a ButtonGroup that defaults to Auto but can be set to Manual, unhiding the slider. I'm sure there are many other options also. But this is also a lot of UI complexity to introduce in the last minute. Bugs around scrollbars, finding a labels that wrap well in multiple languages — these all add up to making this change risky to merge at this point. In hopes of finding the tiniest possible change to what is currently shipping, I created this mock: The only change is that it sets the placeholder text to "Auto" when unset. Despite the impressive work that's gone in, I would ship the latter. This would give us the time to polish and iterate the former, based off the work that's already done. Instead of crunching, the changes suggested can be discussed in a relaxed setting, while still allowing us to strongly mitigate the presently shipping bug. This is not to diminish any of the opinions offered, or any of the work already done. |
@jasmussen Reflecting on this, I'm inclined to agree that the recent revisions -- while worthy improvements explorations -- are too much too late to be trying to include. I agree that scaling back to a more modest iteration over the current behavior of the editor is the best option forward. The placeholder is at least a minimal step to absorb some possible misinterpretations of the empty field. I pushed this in the latest batch of commits. We still need to address the issue of front-end styling for automatic column width growth mentioned in #19515 (comment). |
Avoid situation where adding or removing a Column in a Columns would forcefully assign width to a block which did not previously have a width. Only opt to redistribute widths to explicit numbers if all blocks already have widths.
I pushed another set of changes which aims to try to resolve the issue with front-end styling (it also affected the editor, which I was not previously aware of). The implementation is such that, if a block is not assigned a width, it will maximally grow to occupy whatever space is available. Columns with assigned widths should use that width, and grow no further (same as it was before). I've been trying a handful of different adaptations and I believe it's working correctly. The gutters can sometimes be puzzling to decipher their impact, though I think these are all correct when you consider the impact that more or fewer gutters should have on each individual column: It's much easier to assess when you normalize to remove the gutters: |
@jasmussen I agree that setting the placeholder text to "Auto" when unset would be a valuable change for the release. I would also ask that the second change @aduth proposed which is to display an error message when the total width goes over 100% be added if possible. This is a "must have" to compensate for having a variable percentage associated with the combined width. I would also strongly recommend that the Reset button be re-labelled to represent what it really does which is to reset all the columns in the block. So "Reset Block" or "Reset All" or "Reset Columns" or "Reset all Columns". I leave it yours and @aduth capable hands. we can revisit post release 4.5. I liked the auto / manual per-column selection that @aduth came up with in the end and I like the explicit switch from auto to manual. Let us know if you close this PR and open a new one for post 4.5 discussion. Cheers guys :-) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For me, this works way better than what we currently have. I think we should consider landing this and continuing on any other improvements as follow-ups.
Given the choice between allowing the current behavior to persist through another major version vs. the changes as presented here, I too consider it to be an improvement over the current function of the block. I grant that there remain some potential points of confusion, as discussed and explored for improvement through the history of this pull request. Despite that and in weighing these with the current situation, I believe that the experience of editing columns' widths will be on-the-whole improved. I propose to proceed by:
I recognize the impact that these changes can have, and am very appreciative of all the feedback this has drawn as a result. I'm eager to continue those discussions as we proceed with further improvements to this experience.
The "Reset" button only does affect the width assigned to the currently selected column, so the current label would be accurate. It was a behavior prior to this pull request that pressing "Reset" in a block could have caused the widths of adjacent blocks to have been reset as well, which is no longer the case after these changes. Interestingly, with the previous behavior, you could still have arrived in a scenario of having the same sorts of mixed set of explicitly-assigned and flexible-width column blocks we have talked through here. This would depend on the order in which a user assigns widths, and the position of the block at which "Reset" is pressed (e.g. 3 columns, set 25% on first, then "Reset" on second, result would have been equivalent of |
For posterity's sake, I was intentional in creating the history as retaining original commits of the explorations implemented here (later reversed as revert commits), fully expecting that they may serve as valuable reference for future implementations. See:
In reflecting on the implementation of e25b28b, I observe that it may need additional improvements to handle cases where there is a mixed set of assigned/unassigned widths, where a combination of a subset of those columns would exceed 100% (e.g. |
…columns (#20169) * Block Library: Columns: Fix equal column growth for unassigned-width columns * Block Library: Columns: Restore mid-range viewport styles * Block Library: Columns: Keep margin resets in mid-range viewports Reference from prior to #19515 : https://github.com/WordPress/gutenberg/blob/7997bf66490bab0f6984a114c22ad125f04b9e89/packages/block-library/src/columns/editor.scss#L69-L75 * Block Library: Columns: Update comment to reference mid-range 2 columns
…columns (#20169) * Block Library: Columns: Fix equal column growth for unassigned-width columns * Block Library: Columns: Restore mid-range viewport styles * Block Library: Columns: Keep margin resets in mid-range viewports Reference from prior to #19515 : https://github.com/WordPress/gutenberg/blob/7997bf66490bab0f6984a114c22ad125f04b9e89/packages/block-library/src/columns/editor.scss#L69-L75 * Block Library: Columns: Update comment to reference mid-range 2 columns
…columns (#20169) * Block Library: Columns: Fix equal column growth for unassigned-width columns * Block Library: Columns: Restore mid-range viewport styles * Block Library: Columns: Keep margin resets in mid-range viewports Reference from prior to #19515 : https://github.com/WordPress/gutenberg/blob/7997bf66490bab0f6984a114c22ad125f04b9e89/packages/block-library/src/columns/editor.scss#L69-L75 * Block Library: Columns: Update comment to reference mid-range 2 columns
…columns (#20169) * Block Library: Columns: Fix equal column growth for unassigned-width columns * Block Library: Columns: Restore mid-range viewport styles * Block Library: Columns: Keep margin resets in mid-range viewports Reference from prior to #19515 : https://github.com/WordPress/gutenberg/blob/7997bf66490bab0f6984a114c22ad125f04b9e89/packages/block-library/src/columns/editor.scss#L69-L75 * Block Library: Columns: Update comment to reference mid-range 2 columns
Partially addresses #16370
This pull request seeks to remove auto-adjustment of column widths when updating the width of another column. The intention is to remedy current confusion which can occur when trying to set each column to a specific width, where currently the auto-adjustment can yield an unexpected behavior. With these changes, the auto-adjustment behavior still exists to reallocate space when adding or removing a column from the Columns wrapper block, but specific width updates to individual columns should no longer have any side effects.
This also means there is no longer a guarantee that the sum total of widths of the columns are necessarily equal to 100%. In some cases, this can yield some benefit. For example, it is now fairly simple to create a set of columns which only occupy a fraction of the available space. Additionally, given that columns are not assigned with an explicit width, it allows for columns which are not assigned an explicit width to grow to occupy the available space, rather than forcefully being assigned an explicit width value.
To this last point, this can potentially cause some confusion of its own, since when adjusting the width of a column, you may still observe that the other columns grow or shrink automatically, but only because they are not (yet) assigned with an explicit width, and thus adjust themselves flexibly.
In the discussion of #16370, it was mentioned to consider including a warning notice when the columns were set to widths which did not sum to 100%. This has not yet been implemented in this branch. It could be included here, or as part of a separate pull request, or not at all (depending on feedback to the usability of the proposed changes).
Testing Instructions:
Repeat scenarios from #16370, #19112, or #17918, observing whether you encounter any challenges.