Skip to content

Commit

Permalink
Move normalization step into the dispatch flow (#6549)
Browse files Browse the repository at this point in the history
https://github.com/user-attachments/assets/d184b155-e18a-42d6-ba84-1532f6ebd280

## Problem
The tailwind style plugin can't differentiate between a prop being
removed from the inline style prop, and that prop never having been
there at all. For example, when a flex gap is removed by dragging it in
the negative direction, it goes from positive -> 0 -> being removed from
the style prop. When the normalisation step kicks in, there's no gap
prop in the inline style, so the tailwind plugin doesn't do anything. To
make matters worse, when the gap property is removed while the
interaction is in progress, the value coming from the generated tailwind
class takes precedence, so the flex gap jumps back to its original size.

## Fix
The problem of removing props in the normalization step is fixed by
1. moving the normalization step in the display flow
2. making the normalization step aware that some props need to be
removed
3. find out which elements to normalize/which props to remove from the
dispatched actions and the strategies, and running the normalization on
those elements and prop.

The problem of prevent the CSS classes to kick in when a prop is removed
(for example, when the inline `gap` prop is removed in the flex gap
strategy), this PR adds a postprocessing step alongside the
normalization step, that patches an allowlisted set of props to a "zero"
value, which simulates the prop not being there at all

### Details
- added a test for removing the gap prop set from Tailwind
- added a `propertiesToRemove` param to `normalizeFromInlineStyle`, and
updated the tailwind style plugin to use that prop to remove classes
from the `className` prop
- added the code that finds out which elements/props to normalize from
the dispatched actions
- added the code that finds out which elements/props to normalize from
the strategies, and that tells which elements to patch during an
interaction

**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

Fixes #[ticket_number] (<< pls delete this line if it's not relevant)
  • Loading branch information
bkrmendy authored Oct 17, 2024
1 parent e42de49 commit e053c5b
Show file tree
Hide file tree
Showing 7 changed files with 347 additions and 47 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -772,6 +772,14 @@ export var storyboard = (
const div = editor.renderedDOM.getByTestId(DivTestId)
expect(div.className).toEqual('top-10 left-10 absolute flex flex-row gap-16')
})

it('can remove tailwind gap by dragging past threshold', async () => {
const editor = await renderTestEditorWithModel(Project, 'await-first-dom-report')
await selectComponentsForTest(editor, [EP.fromString('sb/scene/div')])
await doGapResize(editor, canvasPoint({ x: -100, y: 0 }))
const div = editor.renderedDOM.getByTestId(DivTestId)
expect(div.className).toEqual('top-10 left-10 absolute flex flex-row')
})
})
})

Expand Down
5 changes: 4 additions & 1 deletion editor/src/components/canvas/plugins/style-plugins.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,13 +7,16 @@ import {
getTailwindConfigCached,
isTailwindEnabled,
} from '../../../core/tailwind/tailwind-compilation'
import type { PropertiesWithElementPath } from '../../editor/actions/action-utils'
import { isFeatureEnabled } from '../../../utils/feature-switches'

export interface StylePlugin {
name: string
styleInfoFactory: StyleInfoFactory
normalizeFromInlineStyle: (
editorState: EditorState,
elementsToNormalize: ElementPath[],
propertiesToRemove: PropertiesWithElementPath[],
) => EditorState
}

Expand All @@ -23,7 +26,7 @@ export const Plugins = {
} as const

export function getActivePlugin(editorState: EditorState): StylePlugin {
if (isTailwindEnabled()) {
if (isFeatureEnabled('Tailwind') && isTailwindEnabled()) {
return TailwindPlugin(getTailwindConfigCached(editorState))
}
return InlineStylePlugin
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,7 @@ describe('tailwind style plugin', () => {
const normalizedEditor = TailwindPlugin(null).normalizeFromInlineStyle(
editor.getEditorState().editor,
[target],
[],
)

const normalizedElement = getJSXElementFromProjectContents(
Expand Down Expand Up @@ -96,7 +97,7 @@ describe('tailwind style plugin', () => {
const target = EP.fromString('sb/scene/div')
const normalizedEditor = TailwindPlugin(
getTailwindConfigCached(editor.getEditorState().editor),
).normalizeFromInlineStyle(editor.getEditorState().editor, [target])
).normalizeFromInlineStyle(editor.getEditorState().editor, [target], [])

const normalizedElement = getJSXElementFromProjectContents(
target,
Expand Down
77 changes: 64 additions & 13 deletions editor/src/components/canvas/plugins/tailwind-style-plugin.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import * as TailwindClassParser from '@xengine/tailwindcss-class-parser'
import type { JSXAttributesEntry } from 'utopia-shared/src/types'
import type { JSExpression, JSXAttributesEntry, PropertyPath } from 'utopia-shared/src/types'
import { isLeft } from '../../../core/shared/either'
import { getClassNameAttribute } from '../../../core/tailwind/tailwind-options'
import {
Expand All @@ -18,6 +18,7 @@ import type { StylePlugin } from './style-plugins'
import type { WithPropertyTag } from '../canvas-types'
import { withPropertyTag } from '../canvas-types'
import type { Config } from 'tailwindcss/types/config'
import type { PropertiesWithElementPath } from '../../editor/actions/action-utils'

function parseTailwindProperty<T>(value: unknown, parse: Parser<T>): WithPropertyTag<T> | null {
const parsed = parse(value, null)
Expand Down Expand Up @@ -47,7 +48,7 @@ function stringifiedStylePropValue(value: unknown): string | null {
return null
}

function getTailwindClassMapping(classes: string[], config: Config | null) {
function getTailwindClassMapping(classes: string[], config: Config | null): Record<string, string> {
const mapping: Record<string, string> = {}
classes.forEach((className) => {
const parsed = TailwindClassParser.parse(className, config ?? undefined)
Expand All @@ -59,6 +60,55 @@ function getTailwindClassMapping(classes: string[], config: Config | null) {
return mapping
}

function getStylePropContents(
styleProp: JSExpression,
): Array<{ key: string; value: unknown }> | null {
if (styleProp.type === 'ATTRIBUTE_NESTED_OBJECT') {
return mapDropNulls(
(c): { key: string; value: unknown } | null =>
c.type === 'PROPERTY_ASSIGNMENT' &&
c.value.type === 'ATTRIBUTE_VALUE' &&
typeof c.key === 'string'
? { key: c.key, value: c.value.value }
: null,
styleProp.content,
)
}

if (styleProp.type === 'ATTRIBUTE_VALUE' && typeof styleProp.value === 'object') {
return mapDropNulls(
([key, value]) => (typeof key !== 'object' ? null : { key, value }),
Object.entries(styleProp.value),
)
}

return null
}

function getRemoveUpdates(properties: PropertyPath[]): UCL.ClassListUpdate[] {
return mapDropNulls((property) => {
const [maybeStyle, maybeCSSProp] = property.propertyElements
if (
maybeStyle !== 'style' ||
maybeCSSProp == null ||
!isSupportedTailwindProperty(maybeCSSProp)
) {
return null
}
return UCL.remove(TailwindPropertyMapping[maybeCSSProp])
}, properties)
}

function getPropertyCleanupCommands(propertiesToRemove: PropertiesWithElementPath[]) {
return mapDropNulls(({ elementPath, properties }) => {
const removeUpdates = getRemoveUpdates(properties)
if (removeUpdates.length === 0) {
return null
}
return updateClassListCommand('always', elementPath, removeUpdates)
}, propertiesToRemove)
}

export const TailwindPlugin = (config: Config | null): StylePlugin => ({
name: 'Tailwind',
styleInfoFactory:
Expand All @@ -82,7 +132,7 @@ export const TailwindPlugin = (config: Config | null): StylePlugin => ({
),
}
},
normalizeFromInlineStyle: (editorState, elementsToNormalize) => {
normalizeFromInlineStyle: (editorState, elementsToNormalize, propertiesToRemove) => {
const commands = elementsToNormalize.flatMap((elementPath) => {
const element = getJSXElementFromProjectContents(elementPath, editorState.projectContents)
if (element == null) {
Expand All @@ -97,19 +147,15 @@ export const TailwindPlugin = (config: Config | null): StylePlugin => ({
return []
}

const styleValue = styleAttribute.value
if (styleValue.type !== 'ATTRIBUTE_NESTED_OBJECT') {
const styleValue = getStylePropContents(styleAttribute.value)
if (styleValue == null) {
return []
}

const styleProps = mapDropNulls(
(c): { key: keyof typeof TailwindPropertyMapping; value: unknown } | null =>
c.type === 'PROPERTY_ASSIGNMENT' &&
c.value.type === 'ATTRIBUTE_VALUE' &&
isSupportedTailwindProperty(c.key)
? { key: c.key, value: c.value.value }
: null,
styleValue.content,
isSupportedTailwindProperty(c.key) ? { key: c.key, value: c.value } : null,
styleValue,
)

const stylePropConversions = mapDropNulls(({ key, value }) => {
Expand All @@ -134,10 +180,15 @@ export const TailwindPlugin = (config: Config | null): StylePlugin => ({
),
]
})
if (commands.length === 0) {

const commandsWithPropertyCleanup = [
...commands,
...getPropertyCleanupCommands(propertiesToRemove),
]
if (commandsWithPropertyCleanup.length === 0) {
return editorState
}

return foldAndApplyCommandsSimple(editorState, commands)
return foldAndApplyCommandsSimple(editorState, commandsWithPropertyCleanup)
},
})
75 changes: 74 additions & 1 deletion editor/src/components/editor/actions/action-utils.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,6 @@
import { safeIndex } from '../../../core/shared/array-utils'
import type { ElementPath, PropertyPath } from 'utopia-shared/src/types'
import { mapDropNulls, safeIndex } from '../../../core/shared/array-utils'
import type { CanvasCommand } from '../../canvas/commands/commands'
import type { EditorAction } from '../action-types'
import { isFromVSCodeAction } from './actions-from-vscode'

Expand Down Expand Up @@ -362,3 +364,74 @@ export function simpleStringifyActions(
.map((a) => simpleStringifyAction(a, indentation))
.join(`,\n${spacing}`)}\n${spacingBeforeClose}]`
}

export function getElementsToNormalizeFromCommands(commands: CanvasCommand[]): ElementPath[] {
return mapDropNulls((command) => {
switch (command.type) {
case 'ADJUST_CSS_LENGTH_PROPERTY':
case 'SET_CSS_LENGTH_PROPERTY':
case 'ADJUST_NUMBER_PROPERTY':
case 'CONVERT_CSS_PERCENT_TO_PX':
case 'CONVERT_TO_ABSOLUTE':
return command.target
case 'ADD_CONTAIN_LAYOUT_IF_NEEDED':
case 'SET_PROPERTY':
case 'UPDATE_BULK_PROPERTIES':
return command.element
default:
return null
}
}, commands)
}

export function getElementsToNormalizeFromActions(actions: EditorAction[]): ElementPath[] {
return actions.flatMap((action) => {
switch (action.action) {
case 'APPLY_COMMANDS':
return getElementsToNormalizeFromCommands(action.commands)
// TODO: extends this switch when we add support to non-canvas
// command-based edits
default:
return []
}
})
}

export interface PropertiesWithElementPath {
elementPath: ElementPath
properties: PropertyPath[]
}

export function getPropertiesToRemoveFromCommands(
commands: CanvasCommand[],
): PropertiesWithElementPath[] {
return mapDropNulls((command) => {
switch (command.type) {
case 'DELETE_PROPERTIES':
return { elementPath: command.element, properties: command.properties }
case 'UPDATE_BULK_PROPERTIES':
return {
elementPath: command.element,
properties: mapDropNulls(
(p) => (p.type === 'DELETE' ? p.path : null),
command.properties,
),
}
default:
return null
}
}, commands)
}

export function getPropertiesToRemoveFromActions(
actions: EditorAction[],
): PropertiesWithElementPath[] {
return actions.flatMap((action) => {
switch (action.action) {
case 'APPLY_COMMANDS':
return getPropertiesToRemoveFromCommands(action.commands)
default:
return []
}
})
}
Loading

0 comments on commit e053c5b

Please sign in to comment.