try adding max columns to grid #60941
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Size Change: +87 B (0%) Total Size: 1.74 MB
ℹ️ View Unchanged
|
There was a problem hiding this comment.
This is great @SaxonF and is exactly what I was hoping for 😄
One question I have though is whether we're set on using columnCount. I like re-using this option because it feels like it aligns with the idea of these being "constraints" on the grid's behavior. Having the presence of minimumColumnWidth define whether or not it's responsive feels intuitive.
With that said, however, I wonder if we should be cautious of backward incompatibility here. Anyone using both columnCount and minimumColumnWidth would start seeing different behavior. This might be the case if they set layout using the spread operator without correctly clearing the inverse option.
I don't think it's a problem since this behavior feels more likely to be intentional. In my case, for instance, I expected it to work this way in the first place and was surprised that it didn't. It's just important to highlight that as something worth thinking about.
| @@ -351,7 +393,7 @@ function GridLayoutTypeControl( { layout, onChange } ) { | |||
| minimumColumnWidth || '12rem' | |||
| ); | |||
|
|
|||
| const isManual = !! columnCount ? 'manual' : 'auto'; | |||
| const isManual = !! minimumColumnWidth ? 'auto' : 'manual'; | |||
There was a problem hiding this comment.
Nit: Since this isn't a boolean, isManual feels a bit misleading. Maybe something like columnMode? Names are hard 😄
|
Any thoughts on this moving forward @SaxonF @tellthemachines? I'm currently looking at ways to move forward but it depends a bit on this. |
|
@ObliviousHarmony we're days away from feature freeze for WP 6.6 so it's extremely unlikely at this point that this PR will make it in. Would you be able to create a temporary workaround for your use case in the meantime, bearing in mind that this feature will likely ship in 6.7? |
37 tasks
|
@tellthemachines My thinking was we might be able to get away with overriding This is nice because we can make modifications while still benefitting from the wider things we get from The caveat, however, is that I don’t see a similarly easy way to replace the client-side style generation. Another thought I had but haven’t deeply explored yet is the use of a custom layout type. Take the same approach as above but add a custom layout type and related styles. A custom hook on the block can then generate those layout styles. Assuming it doesn’t use the list by default when an invalid type is set (it shouldn’t as far as I could tell), this lets us benefit from some of the same things without needing to try and modify the existing client style rendering. Do you have any other ideas? I’m just really wanting to avoid a Thanks for any insight! |
On the editor side layout is added as a filter to BlockListBlock so you could add your own filter on top of it. If nothing else, the BlockListBlock
This is probably only worth it if you want to add features that are unlikely to ever be built into core. The major downside imo is that once it's being used out in the wild, you'll be stuck with supporting it forever 😅 |
|
Thanks @tellthemachines, this is great guidance.
This should work great! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What?
Adds a maximum column control to grid when in auto mode.
Why?
If we move forward with all grid items being manually positioned when in manual mode, we'll be missing a way to create an adaptive grid with a set number of columns.
How?
Testing Instructions
Testing Instructions for Keyboard
Screenshots or screencast