-
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
Try using manualPlacement
attribute to set manual grid mode and allow responsive behaviour in both modes.
#62777
Changes from 29 commits
2869ebc
c31682c
8cfb76f
4cbbdeb
47d7665
7e06bf8
2a3c600
f552760
c36c2fa
9c3c1dd
e6f033c
82046c4
fd45870
5caa1fa
ee87f98
5dff3ec
fc33eea
ba2aad6
dccead9
dae808e
1a40391
c9f87ba
a0e0ef7
cd81df2
c2b5bed
e0449de
c88a962
2804f30
787c1ca
07876e3
34787ef
fe8cc84
1f957bb
e8be06a
2b30c4f
34b1465
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,5 @@ | ||
https://github.com/WordPress/wordpress-develop/pull/6910 | ||
|
||
* https://github.com/WordPress/gutenberg/pull/59483 | ||
* https://github.com/WordPress/gutenberg/pull/60652 | ||
* https://github.com/WordPress/gutenberg/pull/62777 |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -11,31 +11,22 @@ import { store as blockEditorStore } from '../../store'; | |
import { GridRect } from './utils'; | ||
|
||
export function useGridLayoutSync( { clientId: gridClientId } ) { | ||
const { gridLayout, blockOrder } = useSelect( | ||
( select ) => { | ||
const { getBlockAttributes, getBlockOrder } = | ||
select( blockEditorStore ); | ||
return { | ||
gridLayout: getBlockAttributes( gridClientId ).layout ?? {}, | ||
blockOrder: getBlockOrder( gridClientId ), | ||
}; | ||
}, | ||
[ gridClientId ] | ||
); | ||
|
||
const { getBlockAttributes } = useSelect( blockEditorStore ); | ||
const { getBlockOrder, getBlockAttributes } = useSelect( blockEditorStore ); | ||
const { updateBlockAttributes, __unstableMarkNextChangeAsNotPersistent } = | ||
useDispatch( blockEditorStore ); | ||
|
||
const blockOrder = getBlockOrder( gridClientId ); | ||
|
||
useEffect( () => { | ||
const gridLayout = getBlockAttributes( gridClientId ).layout ?? {}; | ||
const updates = {}; | ||
|
||
const { columnCount, rowCount = 2 } = gridLayout; | ||
const isManualGrid = !! columnCount; | ||
const { columnCount, rowCount, manualPlacement } = gridLayout; | ||
const isManualGrid = !! manualPlacement; | ||
|
||
if ( isManualGrid ) { | ||
const rects = []; | ||
let cellsTaken = 0; | ||
let lowestRowEnd = 0; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is a somewhat confusing name because "lowest row end" could mean the bottom-most row or the smallest "row end". There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
|
||
// Respect the position of blocks that already have a columnStart and rowStart value. | ||
for ( const clientId of blockOrder ) { | ||
|
@@ -58,18 +49,11 @@ export function useGridLayoutSync( { clientId: gridClientId } ) { | |
rowSpan, | ||
} ) | ||
); | ||
lowestRowEnd = Math.max( lowestRowEnd, rowStart + rowSpan - 1 ); | ||
} | ||
|
||
// Ensure there's enough rows to fit all blocks. | ||
const minimumNeededRows = Math.ceil( cellsTaken / columnCount ); | ||
if ( rowCount < minimumNeededRows ) { | ||
updates[ gridClientId ] = { | ||
layout: { | ||
...gridLayout, | ||
rowCount: minimumNeededRows, | ||
}, | ||
}; | ||
} | ||
|
||
// When in manual mode, ensure that every block has a columnStart and rowStart value. | ||
for ( const clientId of blockOrder ) { | ||
|
@@ -104,6 +88,15 @@ export function useGridLayoutSync( { clientId: gridClientId } ) { | |
}, | ||
}, | ||
}; | ||
lowestRowEnd = Math.max( lowestRowEnd, rowStart + rowSpan - 1 ); | ||
} | ||
if ( rowCount !== lowestRowEnd ) { | ||
updates[ gridClientId ] = { | ||
layout: { | ||
...gridLayout, | ||
rowCount: minimumNeededRows, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this supposed to be |
||
}, | ||
}; | ||
} | ||
} else { | ||
// When in auto mode, remove all of the columnStart and rowStart values. | ||
|
@@ -121,6 +114,15 @@ export function useGridLayoutSync( { clientId: gridClientId } ) { | |
}; | ||
} | ||
} | ||
// Remove row styles in auto mode | ||
if ( rowCount ) { | ||
updates[ gridClientId ] = { | ||
layout: { | ||
...gridLayout, | ||
rowCount: undefined, | ||
}, | ||
}; | ||
} | ||
} | ||
|
||
if ( Object.keys( updates ).length ) { | ||
|
@@ -134,7 +136,6 @@ export function useGridLayoutSync( { clientId: gridClientId } ) { | |
}, [ | ||
// Actual deps to sync: | ||
gridClientId, | ||
gridLayout, | ||
blockOrder, | ||
// Needed for linter: | ||
__unstableMarkNextChangeAsNotPersistent, | ||
|
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.
Why are we now calling
getBlockOrder
outside of theuseSelect
? If we want it to be a dep of theuseEffect
then it needs to be called inside theuseSelect
so that it updates when the store is updated.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.
It doesn't seem to make a difference - on trunk, if I add a new block to a grid by pressing enter on a Paragraph block already inside it, the effect doesn't re-run. That might be due to this hook rendering inside the grid component. Ideally we get it to update whenever a grid child is added or updated, but I think that'll require bigger changes.
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.
I think you're right. We'll need to move
<GridLayoutSync>
out oftoolBarControls
and into a component that's not just rendered when the grid block is selected.Regardless, I don't think this PR should be moving
getBlockOrder
outside of theuseSelect
😀 If it were working on trunk I'd expect this change to break it.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.
I've reverted this for now and will move the hook in the PR that separates the visualizer out from the grid toolbar (working branch here, still very early stages)