-
Notifications
You must be signed in to change notification settings - Fork 171
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
Set props through style plugin #6599
Conversation
#15079 Bundle Size — 58.06MiB (~-0.01%).badf4e9(current) vs d1d46c1 master#15078(baseline) Warning Bundle contains 70 duplicate packages – View duplicate packages Bundle metrics
|
Current #15079 |
Baseline #15078 |
|
---|---|---|
Initial JS | 41.04MiB (~-0.01% ) |
41.04MiB |
Initial CSS | 0B |
0B |
Cache Invalidation | 18.11% |
18% |
Chunks | 20 |
20 |
Assets | 22 |
22 |
Modules | 4167 |
4167 |
Duplicate Modules | 213 |
213 |
Duplicate Code | 27.3% |
27.3% |
Packages | 477 |
477 |
Duplicate Packages | 70 |
70 |
Bundle size by type 2 changes
2 improvements
Current #15079 |
Baseline #15078 |
|
---|---|---|
JS | 58.06MiB (~-0.01% ) |
58.06MiB |
HTML | 7.37KiB (-0.25% ) |
7.39KiB |
Bundle analysis report Branch feature/set-prop-through-plugin Project dashboard
Generated by RelativeCI Documentation Report issue
editor/src/components/canvas/canvas-strategies/canvas-strategies.tsx
Outdated
Show resolved
Hide resolved
editor/src/components/canvas/commands/delete-properties-command.ts
Outdated
Show resolved
Hide resolved
@@ -1471,6 +1472,7 @@ export interface EditorState { | |||
collaborators: Collaborator[] | |||
sharingDialogOpen: boolean | |||
editorRemixConfig: EditorRemixConfig | |||
propertiesUpdatedDuringInteraction: UpdatedProperties |
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.
isn't this leftover?
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's very much not a leftover, this prop is read by these functions
utopia/editor/src/components/canvas/plugins/style-plugins.ts
Lines 77 to 83 in 0908603
export function resetUpdatedProperties(editorState: EditorState): EditorState { | |
return { ...editorState, propertiesUpdatedDuringInteraction: {} } | |
} | |
function getPropertiesUpdatedDuringInteraction(editorState: EditorState) { | |
return editorState.propertiesUpdatedDuringInteraction | |
} |
utopia/editor/src/components/canvas/plugins/style-plugins.ts
Lines 132 to 138 in 0908603
return { | |
editorStateWithChanges: editorStateWithChanges, | |
editorStatePatches: [ | |
editorStatePatch, | |
{ propertiesUpdatedDuringInteraction: { $set: updatedProperties } }, | |
], | |
} |
why doesn't the |
…to feature/set-prop-through-plugin
We talked about this IRL, the reason this is needed is because the code that does the zeroing for padding needs to know about all the props that have been set/unset (see this code from the spike) |
See #6574 for a long-form writeup on the full background of this PR. ## The problem addressed on this PR The normalisation step cannot easily handle commands/actions removing properties from elements. ## Fix Instead of working around the limitation of the normalisation step, this PR introduces an approach where the style plugins themselves implement setting/removing style props on elements, and the `SetProperty` / `DeleteProperty` command uses the style plugin API to set/remove style props. The PR deals with patching props that are removed during a canvas interaction (details in #6574, TLDR: if `gap` is removed with the gap control, it needs to be set to `0px` while the interaction is in progress, otherwise the value from the Tailwind class will be applied which would be janky). This is an under-the-hood PR, no user-facing functionality is added. ### Details - Updated the `StylePlugin` interface with the `updateStyles` function, which can be used to set/delete element styles. The inline style plugin and the Tailwind style plugin are updated accordingly - Injected `StylePlugin.updateStyles` into `runSetProperty` and `runDeleteProperties` via the new `runStyleUpdateForStrategy` function. This function encapsulates a performance-related feature: if an interaction is ongoing, it sets props in the inline style prop (and patches removed props), so updates are real-time, and when an interaction is done, it uses the active plugin. This trick works because the active strategy is re-run on interaction finish, producing the right patches for a style update. - The machinery dealing with zeroing props is added. The entrypoint for adding the zeroed props is `patchRemovedProperties` (called from `dispatch-strategies.tsx`, and the entrypoint for recording which props need to be zeroed is `runStyleUpdateMidInteraction` - The code related to the normalisation step is removed - `UpdateClassListCommand` is removed and turned into a helper function (since it's not called as a command anymore, just as a helper in the Tailwind style plugin) **Manual Tests:** I hereby swear that: - [x] I opened a hydrogen project and it loaded - [x] I could navigate to various routes in Play mode
See #6574 for a long-form writeup on the full background of this PR.
The problem addressed on this PR
The normalisation step cannot easily handle commands/actions removing properties from elements.
Fix
Instead of working around the limitation of the normalisation step, this PR introduces an approach where the style plugins themselves implement setting/removing style props on elements, and the
SetProperty
/DeleteProperty
command uses the style plugin API to set/remove style props. The PR deals with patching props that are removed during a canvas interaction (details in #6574, TLDR: ifgap
is removed with the gap control, it needs to be set to0px
while the interaction is in progress, otherwise the value from the Tailwind class will be applied which would be janky).This is an under-the-hood PR, no user-facing functionality is added.
Details
StylePlugin
interface with theupdateStyles
function, which can be used to set/delete element styles. The inline style plugin and the Tailwind style plugin are updated accordinglyStylePlugin.updateStyles
intorunSetProperty
andrunDeleteProperties
via the newrunStyleUpdateForStrategy
function. This function encapsulates a performance-related feature: if an interaction is ongoing, it sets props in the inline style prop (and patches removed props), so updates are real-time, and when an interaction is done, it uses the active plugin. This trick works because the active strategy is re-run on interaction finish, producing the right patches for a style update.patchRemovedProperties
(called fromdispatch-strategies.tsx
, and the entrypoint for recording which props need to be zeroed isrunStyleUpdateMidInteraction
UpdateClassListCommand
is removed and turned into a helper function (since it's not called as a command anymore, just as a helper in the Tailwind style plugin)Manual Tests:
I hereby swear that: