Optimise block rendering performance #150
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR implements performance optimizations for block rendering in the Block Grid when moving, resizing, or changing data. The changes introduce more selective re-rendering logic to avoid full re-renders during block movements, while maintaining necessary re-renders for content and dimension changes.
Key changes include:
- Enhanced context observation to track column and row span changes
- Improved pointer event handling for drag detection to prevent accidental edit triggers
- Optimized rendering lifecycle with
willUpdatereplacingupdatedfor better control
Reviewed Changes
Copilot reviewed 7 out of 12 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| sort-mode.property-action-anRi0zXU.js | Updated import references for bundled file changes |
| index-D1Fdp__Z.js | Main bundle containing optimized block grid preview logic with selective rendering |
| block-list-sort-mode-CmBn_tm2.js | Updated import path for sort mode property action |
| block-grid-sort-mode-DuwCHXT-.js | Updated import path for sort mode property action |
| assets.js | Updated import path for main index bundle |
| block-grid-preview.custom-view.element.ts | Core TypeScript source with performance optimizations |
| services.config | Updated content data structure for block grid configuration |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| if (!this._isFirstLoad && !_changedProperties.has('_isFirstLoad') && !_changedProperties.has('_isLoading')) { | ||
| if ( | ||
| (_changedProperties.has('content') && this.content) || | ||
| (_changedProperties.has('settings') && this.settings)) { | ||
| this.#renderBlockPreview(); | ||
| } | ||
|
|
||
| if (_changedProperties.has('content') || _changedProperties.has('settings')) { | ||
| if (this._previewTimeout) { | ||
| clearTimeout(this._previewTimeout); | ||
| if (_changedProperties.has('blockGridValue') || | ||
| (_changedProperties.has('_columnSpan') && this._columnSpan) || | ||
| (_changedProperties.has('_rowSpan') && this._rowSpan)) { | ||
| const value = _changedProperties.get('blockGridValue') as UmbBlockGridValueModel | undefined; | ||
| if (value) { | ||
| const layouts = value.layout ? value.layout['Umbraco.BlockGrid'] : undefined; | ||
| if (layouts) { | ||
| const newColumnSpan = this._getColumnSpan(layouts); | ||
| const newRowSpan = this._getRowSpan(layouts); | ||
|
|
||
| if (newColumnSpan !== this._columnSpan || newRowSpan !== this._rowSpan) { | ||
| this._columnSpan = newColumnSpan; | ||
| this._rowSpan = newRowSpan; | ||
| this.#renderBlockPreview(); | ||
| } |
There was a problem hiding this comment.
[nitpick] Consider adding early return conditions to minimize unnecessary work in willUpdate. The current implementation processes multiple conditions even when _isFirstLoad is true, which could be optimized by checking this flag earlier.
| if (!this._isFirstLoad && !_changedProperties.has('_isFirstLoad') && !_changedProperties.has('_isLoading')) { | ||
| if ( | ||
| (_changedProperties.has('content') && this.content) || | ||
| (_changedProperties.has('settings') && this.settings)) { | ||
| this.#renderBlockPreview(); | ||
| } |
There was a problem hiding this comment.
[nitpick] The nested if conditions create complex control flow. Consider extracting this logic into a private method like #shouldRenderOnContentChange() to improve readability and maintainability.
| this._pointerDownPos = { x: event.clientX, y: event.clientY }; | ||
| this._isDragging = false; | ||
| } | ||
|
|
||
| _handlePointerMove(event: PointerEvent) { | ||
| if (this._pointerDownPos) { |
There was a problem hiding this comment.
[nitpick] Consider adding null checks and error handling for the PointerEvent properties. While unlikely to be null in modern browsers, defensive programming would prevent potential runtime errors.
| this._pointerDownPos = { x: event.clientX, y: event.clientY }; | |
| this._isDragging = false; | |
| } | |
| _handlePointerMove(event: PointerEvent) { | |
| if (this._pointerDownPos) { | |
| if (typeof event.clientX === 'number' && typeof event.clientY === 'number') { | |
| this._pointerDownPos = { x: event.clientX, y: event.clientY }; | |
| } else { | |
| this._pointerDownPos = null; | |
| console.warn('PointerEvent missing clientX/clientY:', event); | |
| } | |
| this._isDragging = false; | |
| } | |
| _handlePointerMove(event: PointerEvent) { | |
| if (this._pointerDownPos) { | |
| if (typeof event.clientX !== 'number' || typeof event.clientY !== 'number') { | |
| console.warn('PointerEvent missing clientX/clientY:', event); | |
| return; | |
| } |
| _handlePointerMove(event: PointerEvent) { | ||
| if (this._pointerDownPos) { | ||
| const deltaX = Math.abs(event.clientX - this._pointerDownPos.x); | ||
| const deltaY = Math.abs(event.clientY - this._pointerDownPos.y); | ||
| if (deltaX > 5 || deltaY > 5) { | ||
| this._isDragging = true; | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
[nitpick] The drag threshold value of 5 pixels should be extracted to a class constant or configuration property to make it easily adjustable and self-documenting.
|
rickbutterfield
changed the title
|
An attempt to improve performance of blocks in the Block Grid when moving, resizing or changing data. A full re-render should not be necessary when moving blocks, but may be necessary when resizing or changing the content/settings of the block.
Fixes #149