Simplified layout multi-select support. #4376
Merged
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
- Moved `changeBounds` and `ChangeBoundsResult` to `shared-absolute-resize-strategy-helpers.ts`. - Added `resizeInspectorStrategy` and `directResizeInspectorStrategy`. - Extracted out `getAppropriateLocalFrame` as the logic was needed/used in more than one place. - Implemented `getDirectMoveCommandsForSelectedElement`. - Added `startingFeatureSwitches` parameter to `renderTestEditorWithCode`. - Changed `originalLocalFrame` to `originalLTWHValues` so that we can instead capture plurality of the positional/sizing properties. - Added a `FrameUpdate` type to the simplified layout section to capture delta changes versus direct (where the property is set to a specific value) changes. - `updateFrame` callback extended to support the differing types of updates by executing differing inspector strategies.
seanparsons
requested review from
gbalint,
Rheeseyb,
ruggi,
balazsbajorics,
enidemi and
bkrmendy
Job #8693: Bundle Size — 63.29MiB (+0.02%).
Warning Bundle contains 64 duplicate packages – View duplicate packages Bundle metrics
 
Bundle size by type
View job #8693 report View feature/multiselect-layout-pins-... branch activity |
|
Performance test results: |
| const edgePosition: EdgePosition = | ||
| widthOrHeight === 'width' ? EdgePositionRight : EdgePositionBottom | ||
| const elementMetadata = MetadataUtils.findElementByElementPath(metadata, selectedElement) | ||
| const originalFrame = elementMetadata?.globalFrame |
There was a problem hiding this comment.
should this be the localFrame instead? are we using localFrame anywhere anymore? – edit: for size currently the localFrame and globalFrame matches, currently... that is until we start adding scaling or rotation support!
There was a problem hiding this comment.
I went back and forth a little on this, IIRC the pre-existing code pushed me more in the direction of the global canvas frame.
editor/src/components/canvas/canvas-strategies/strategies/shared-move-strategies-helpers.ts
Show resolved
Hide resolved
...ctions/layout-section/self-layout-subsection/frame-updating-layout-section.spec.browser2.tsx
Show resolved
Hide resolved
Rheeseyb
reviewed
Oct 17, 2023
...ctions/layout-section/self-layout-subsection/frame-updating-layout-section.spec.browser2.tsx
Outdated
Show resolved
Hide resolved
Rheeseyb
approved these changes
Oct 17, 2023
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.
Problem:
The new simplified layout section currently doesn't support updates for multiselection when ideally it should.
Fix:
Behind the scenes of the new layout section, changes have been implemented via delta updates at a high level. With single selection it's easy to take the current value and given a new value determine the delta and then feed that down into the inspector strategy. However that doesn't work as easily when handling multiselected elements with mixed values for the same property and would require a lot of that logic to be handled in the UI layer which also seems like the wrong approach.
In this PR that translation is handled by "direct" inspector strategies which layer over the regular delta based strategies, taking a particular value, calculating the delta and passing it into the logic previously handling the purely delta changes (at least for resizing).
Given the strategies were previously based on the deltas, this resulted in comparatively little changes as we've been able to reuse the logic, but it does seem a bit odd to have the direct value and then calculate the delta, which then calculates the direct value lower down.
Commit Details:
changeBoundsandChangeBoundsResulttoshared-absolute-resize-strategy-helpers.ts.resizeInspectorStrategyanddirectResizeInspectorStrategy.getAppropriateLocalFrameas the logic was needed/used in more than one place.getDirectMoveCommandsForSelectedElement.startingFeatureSwitchesparameter torenderTestEditorWithCode.originalLocalFrametooriginalLTWHValuesso that we can instead capture plurality of the positional/sizing properties.FrameUpdatetype to the simplified layout section to capture delta changes versus direct (where the property is set to a specific value) changes.updateFramecallback extended to support the differing types of updates by executing differing inspector strategies.