Grid Visualizer - improve styles#64321
Conversation
…ld. Use context to provide resizer bounds
talldan
added
[Block] Group
|
Size Change: +1.45 kB (+0.08%) Total Size: 1.77 MB
ℹ️ View Unchanged
|
talldan
force-pushed
the
try/inline-grid-visualizer
branch
from
88002c5 to
6c3b96a
Compare
…ne over has all the interactive elements
I've pushed a few more commits that try this, and it works pretty well, it partially solves the issue with the cells inside the editor iframe not having access to component CSS, as now the most interactive parts of the grid visualizer are outside the canvas, and only the cells underneath the grid need some basic styles which might be as simple as inlines styles. |
|
This is cool. I'll take it for a spin.
Do you think it's still worth doing this? Or are the styles minimal enough now that it doesn't matter? It's a problem we run into semi frequently. Might be fun to build an |
|
This is really cool—you're a genius. Can we hide the drop zone / inserter on occupied cells when you're not dragging so that you don't see stripes on top of blocks? 
|
I was wondering why some cells had a slightly different color. 😄 I've pushed a commit that does that, and also a bunch more styling updates to make it look more like the video in #64162 (comment). I'm mostly doing this by sight, but I think it's pretty close. I'll work a bit more on the highlighted cell styles. Then I probably need to tidy up some before it's ready for review. I'm surprised how quickly this has come together. That the popover component now properly supports being inline helps a lot. |
|
Flaky tests detected in 4620f5e. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/10367423474
|
| shift = false, | ||
| inline = false, | ||
| variant, | ||
| contentStyle, |
There was a problem hiding this comment.
Probably need to spin the popover changes out into a separate PR. Or look at alternative options for popover styles inside the canvas.
There was a problem hiding this comment.
Could it be done in the style.scss? I suppose not because it's in the iframe.
There was a problem hiding this comment.
Yep, that's correct, it won't work because of the iframe. Two of the the options are to use inline styles or render a stylesheet.
Another would be to render the BlockPopoverCover inside <div> and use that div to reset the margin, but there'd still be the popover z-index to unset. Maybe though the components team would be more open to a noZIndex prop or something then though. I'll find out.
talldan
requested review from
ajitbohra,
ellatrix and
tellthemachines
as code owners
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
talldan
changed the title
talldan
added
[Type] Enhancement
noisysocks
left a comment
noisysocks
left a comment
There was a problem hiding this comment.
Love this, it's a big improvement in UI and code quality.
| /> | ||
| { isManualGrid && ( | ||
| <GridPopover | ||
| ref={ ref } |
There was a problem hiding this comment.
There's a bug here caused by ref only being passed/set when isManualGrid is true. It means that you're able to resize a grid item beyond the bounds of the grid block when it's in auto mode.
Kapture.2024-08-12.at.15.42.13.mp4
There was a problem hiding this comment.
Should be fixed in f73ca9f. The annoying aspect is that it means some parts of GridPopover always have to be rendered even when there's not much going on, but I think it's necessary. I tried to optimize it by moving the items into a separate component.
To properly finish #64162, the auto grid will also need a single appender in the next available cell, so something like this will be needed anyway.
There was a problem hiding this comment.
To properly finish #64162, the auto grid will also need a single appender in the next available cell, so something like this will be needed anyway.
I think we can ignore that tbh. It's better imo to have it consistent across container blocks.
| box-shadow: none; | ||
| background-color: rgba(var(--wp-admin-theme-color--rgb), 0.05); | ||
| border: 1px dashed; | ||
| border-radius: 2px; |
There was a problem hiding this comment.
I think we have a $border-radius var for this.
There was a problem hiding this comment.
Oh yep, that was lazy. Now updated.
| const addGridVisualizerToBlockEdit = createHigherOrderComponent( | ||
| ( BlockEdit ) => ( props ) => { | ||
| // Use useState() instead of useRef() so that GridItemResizer updates when ref is set. | ||
| const [ resizerBounds, setResizerBounds ] = useState(); |
There was a problem hiding this comment.
Maybe move this useState and the Provider to a new component so that we're not calling useState on every single block regardless of whether it has a grid layout. Probably doesn't make any difference to performance in practice but I think it's a nice habit to be really careful about what goes above the early return in a BlockEdit filter.
There was a problem hiding this comment.
Good point, I think it's also much tidier to move it into GridTools and have GridTools accept children. Done in 6139488
| <GridVisualizer | ||
| clientId={ rootClientId } | ||
| contentRef={ setResizerBounds } | ||
| parentLayout={ parentLayout } | ||
| /> |
There was a problem hiding this comment.
I like this approach regardless. Nice to have only one instance of GridVisualizer.
| shift = false, | ||
| inline = false, | ||
| variant, | ||
| contentStyle, |
There was a problem hiding this comment.
Could it be done in the style.scss? I suppose not because it's in the iframe.
| return ( | ||
| <BlockPopoverCover | ||
| __unstablePopoverSlot="__unstable-block-tools-after" | ||
| className={ clsx( 'block-editor-grid-visualizer', { |
There was a problem hiding this comment.
The class names in this file have come out of sync with the component names. It used to be that GridVisualizer rendered an element with className = 'block-editor-grid-visualizer' and so on. Should we update them?
Also, is it bad that both BlockPopover and BlockPopunder render an element with the same classname?
There was a problem hiding this comment.
Good point. I've pushed a commit that updates these. GridPopunder doesn't need classnames it relies on the inline css, so removing those solves the duplication. I changed the GridPopover classnames to block-editor-grid-visualizer-popover to match the components.
Also removed another unused classname.
It's not perfect, but I think good enough right now.
- Remove unused classnames - Update existing classnames to use BEM component convention
…ake `GridTools` accept children.
|
I've made a separate PR for the popover changes - Components: Allow style prop on Popover |
noisysocks
added
the
[Feature] Layout
|
Unfortunately I've found a couple of bugs that may be difficult to solve. Rendering the visualizer popover inline before the grid means layout rules can apply to it, like this one: .is-layout-constrained > :first-child {
margin-block-start: 0;
}The problem is not so much that the layout rule applies to the visualizer - it can be overridden, more that it no longer applies to the grid block itself like its supposed to. It results in the layout being incorrect. The popover also only renders when the grid is selected, and that causes a layout shift as the css rule jumps from applying to the block to suddenly applying to the popover: Kapture.2024-08-15.at.10.42.55.mp4The second issue is that grid block's background can cover the visualizer popover. Rendering the visualizer popover inside the Grid block is probably the next best option, though there's no easy way to do it using a filter from what I understand 🤔 |
Can we modify the |
|
Or |
|
Or maybe render a slot in the selected grid block. It'd be implemented in the group block, but the slot/fill could be a private API from the block editor package. 🤔 |
|
Just wondering if this PR still valid or worth reviving? |
|
I've tried rebasing it, and the conflicts are very hard to resolve, so I'll close it. |
3 participants
What?
See #64162
Brings the styles of the grid visualizer closer to those shown in the video on the issue.
There are a few bits this doesn't do from the video:
There are a few other subtleties that might worth trying in separate PRs:
How?
In order to achieve the visual styling shown in #64162, the grid visualizer needs to be rendered behind the grid block.
This PR attempts this using an inline popover.
It requires quite a bit of code-reshuffling as there are now two separate grid visualizers being rendered:
GridPopunder- renders a visualizer under the selected grid block. This visualizer is entirely visual, it renders some blue rectangles.GridPopover- renders interactive elements over the selected block. This is responsible for the drop zones and appenders in the grid. It has to be over the selected block to receive mouse events.Testing Instructions
Screenshots or screencast