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.

Sign up for GitHub

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

Grid interactivity: Improve how grid resizer handles 0-width and 0-height cells #61423

Merged
merged 2 commits into from
May 9, 2024
Merged

Grid interactivity: Improve how grid resizer handles 0-width and 0-height cells #61423

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
3c110e8
Grid interactivity: Improve how grid resizer handles 0-width and 0-he…
noisysocks May 7, 2024
0a0a87c
Add doc comments
noisysocks May 9, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
68 changes: 38 additions & 30 deletions packages/block-editor/src/components/grid-visualizer/grid-item-resizer.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,54 +45,62 @@ export function GridItemResizer( { clientId, onChange } ) {
const rowGap = parseFloat(
getComputedCSS( gridElement, 'row-gap' )
);
const gridColumnLines = getGridLines(
const gridColumnTracks = getGridTracks(
getComputedCSS( gridElement, 'grid-template-columns' ),
columnGap
);
const gridRowLines = getGridLines(
const gridRowTracks = getGridTracks(
getComputedCSS( gridElement, 'grid-template-rows' ),
rowGap
);
const columnStart = getClosestLine(
gridColumnLines,
blockElement.offsetLeft
);
const rowStart = getClosestLine(
gridRowLines,
blockElement.offsetTop
);
const columnEnd = getClosestLine(
gridColumnLines,
blockElement.offsetLeft + boxElement.offsetWidth
);
const rowEnd = getClosestLine(
gridRowLines,
blockElement.offsetTop + boxElement.offsetHeight
);
const columnStart =
getClosestTrack(
gridColumnTracks,
blockElement.offsetLeft
) + 1;
const rowStart =
getClosestTrack(
gridRowTracks,
blockElement.offsetTop
) + 1;
const columnEnd =
getClosestTrack(
gridColumnTracks,
blockElement.offsetLeft + boxElement.offsetWidth,
'end'
) + 1;
const rowEnd =
getClosestTrack(
gridRowTracks,
blockElement.offsetTop + boxElement.offsetHeight,
'end'
) + 1;
onChange( {
columnSpan: Math.max( columnEnd - columnStart, 1 ),
rowSpan: Math.max( rowEnd - rowStart, 1 ),
columnSpan: columnEnd - columnStart + 1,
rowSpan: rowEnd - rowStart + 1,
} );
} }
/>
</BlockPopoverCover>
);
}

function getGridLines( template, gap ) {
const lines = [ 0 ];
function getGridTracks( template, gap ) {
const tracks = [];
for ( const size of template.split( ' ' ) ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Not to worry about for this PR, but just wondering if we'll ever need to handle templates with more complex values for each of the columns or rows? I.e. if a function is used like minmax then the space character would be between part of a function value instead of between each column value. We don't yet support complex values, so I don't think there's anything we need to do to handle it just yet, but thought I'd mention it in case we need a safe escape point / bail early if the values aren't what we expect.

Copy link
Contributor

Choose a reason for hiding this comment

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

That won't be a problem as long as the values we pass to this function come from getComputedCSS, because computed values will all be in px. I guess it might be good to make it clear that that's what this function expects 😅

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, excellent. Thanks for explaining!

const line = parseFloat( size );
lines.push( lines[ lines.length - 1 ] + line + gap );
const previousTrack = tracks[ tracks.length - 1 ];
const start = previousTrack ? previousTrack.end + gap : 0;
const end = start + parseFloat( size );
tracks.push( { start, end } );
}
return lines;
return tracks;
}

function getClosestLine( lines, position ) {
return lines.reduce(
( closest, line, index ) =>
Math.abs( line - position ) <
Math.abs( lines[ closest ] - position )
function getClosestTrack( tracks, position, edge = 'start' ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it worth adding unit tests for these two functions? Being unfamiliar with this code it took me a little while to wrap my head around what it's doing (grabbing a computed CSS value, splitting it into parts, and then providing values for distances to closest edge). I think I'd struggle a little bit attempting to debug or tweak the logic if I had to, so wondered if a couple of tests might help document the expected behaviour.

Of course, I understand this is still experimental and the feature is in flux, so no worries if you think it's too early to lock the behaviour in place!

Copy link
Contributor

Choose a reason for hiding this comment

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

Unit tests would be good at some point but I think what would also be good for clarity would be a docblock. Too many of our internal functions aren't documented at all, and even just a line describing what it does, parameters and return values can be super helpful!

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, a docblock comment would be great! Especially while you're still moving things around, I reckon a little documentation is probably quicker and easier than unit tests at this point 👍

return tracks.reduce(
( closest, track, index ) =>
Math.abs( track[ edge ] - position ) <
Math.abs( tracks[ closest ][ edge ] - position )
? index
: closest,
0
Expand Down
Loading