Skip to content

Commit

Permalink
Update SetCSSLengthCommand to read from StyleInfo (#6651)
Browse files Browse the repository at this point in the history
## Problem
The flex section relies on the `SetCSSLengthCommand` command to update
elements, both for reading and writing styles. In order for that to work
in Tailwind projects, where elements don't have an inline style prop,
`SetCSSLengthCommand` needs to read element styles from the "right"
property, otherwise the command wouldn't work as intended.

## Fix
Use the StyleInfo system from to read the element style info.

Details
- Refactored `SetCSSLengthCommand` to read elements styles through
styleInfo
- Created a bespoke prop, `CSSLengthProperty`, to track which props are
addressed by `SetCSSLengthCommand`. This way it's easy to tell which
props need to be supported by StyleInfo to have `SetCSSLengthCommand`
working well
- Extended StyleInfo to support `zIndex` (all other props in
`CSSLengthProperty` were already supported by `StyleInfo`
- Updated the style plugins to support the new StyleInfo props
- update the signatures of `fixlengthCommand` and
`getCSSNumberFromStyleInfo` to use narrower type for some of their
arguments

**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
  • Loading branch information
bkrmendy authored Nov 18, 2024
1 parent 21678a3 commit 17c6983
Show file tree
Hide file tree
Showing 7 changed files with 50 additions and 60 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ import {
trueUpChildrenOfGroupChanged,
} from '../../../../components/editor/store/editor-state'
import { getLayoutProperty } from '../../../../core/layout/getLayoutProperty'
import type { StyleLayoutProp } from '../../../../core/layout/layout-helpers-new'
import type { LayoutPinnedProp, StyleLayoutProp } from '../../../../core/layout/layout-helpers-new'
import type { PropsOrJSXAttributes } from '../../../../core/model/element-metadata-utils'
import {
MetadataUtils,
Expand Down Expand Up @@ -551,7 +551,7 @@ export function isEmptyGroup(metadata: ElementInstanceMetadataMap, path: Element
)
}

function fixLengthCommand(path: ElementPath, prop: StyleLayoutProp, size: number): CanvasCommand {
function fixLengthCommand(path: ElementPath, prop: LayoutPinnedProp, size: number): CanvasCommand {
return setCssLengthProperty(
'always',
path,
Expand Down
3 changes: 3 additions & 0 deletions editor/src/components/canvas/canvas-types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -586,6 +586,7 @@ export type HeightInfo = CSSStyleProperty<CSSNumber>
export type FlexBasisInfo = CSSStyleProperty<CSSNumber>
export type PaddingInfo = CSSStyleProperty<CSSPadding>
export type PaddingSideInfo = CSSStyleProperty<CSSNumber>
export type ZIndexInfo = CSSStyleProperty<CSSNumber>

export interface StyleInfo {
gap: FlexGapInfo | null
Expand All @@ -602,6 +603,7 @@ export interface StyleInfo {
paddingRight: PaddingSideInfo | null
paddingBottom: PaddingSideInfo | null
paddingLeft: PaddingSideInfo | null
zIndex: ZIndexInfo | null
}

const emptyStyleInfo: StyleInfo = {
Expand All @@ -619,6 +621,7 @@ const emptyStyleInfo: StyleInfo = {
paddingRight: null,
paddingBottom: null,
paddingLeft: null,
zIndex: null,
}

export const isStyleInfoKey = (key: string): key is keyof StyleInfo => key in emptyStyleInfo
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ import type { StyleInfo } from '../canvas-types'

export type CreateIfNotExistant = 'create-if-not-existing' | 'do-not-create-if-doesnt-exist'

type LengthProperty = 'left' | 'right' | 'bottom' | 'top' | 'width' | 'height' | 'flexBasis'
export type LengthProperty = 'left' | 'right' | 'bottom' | 'top' | 'width' | 'height' | 'flexBasis'

type LengthPropertyPath = PropertyPath<['style', LengthProperty]>

Expand Down
91 changes: 39 additions & 52 deletions editor/src/components/canvas/commands/set-css-length-command.ts
Original file line number Diff line number Diff line change
@@ -1,34 +1,26 @@
import { notice } from '../../../components/common/notice'
import { isLeft, isRight, left } from '../../../core/shared/either'
import * as EP from '../../../core/shared/element-path'
import {
emptyComments,
isJSXElement,
jsExpressionValue,
} from '../../../core/shared/element-template'
import type { GetModifiableAttributeResult, ValueAtPath } from '../../../core/shared/jsx-attributes'
import {
getModifiableJSXAttributeAtPath,
jsxSimpleAttributeToValue,
} from '../../../core/shared/jsx-attribute-utils'
import { roundTo } from '../../../core/shared/math-utils'
import type { ElementPath, PropertyPath } from '../../../core/shared/project-file-types'
import * as PP from '../../../core/shared/property-path'
import type { EditorState, EditorStatePatch } from '../../editor/store/editor-state'
import { withUnderlyingTargetFromEditorState } from '../../editor/store/editor-state'
import type { EditorState } from '../../editor/store/editor-state'
import type { CSSKeyword, CSSNumber, FlexDirection } from '../../inspector/common/css-utils'
import {
cssPixelLength,
parseCSSPercent,
printCSSNumber,
printCSSNumberOrKeyword,
} from '../../inspector/common/css-utils'
import type { CreateIfNotExistant } from './adjust-css-length-command'
import { deleteConflictingPropsForWidthHeight } from './adjust-css-length-command'
import type { LengthProperty } from './adjust-css-length-command'
import {
deleteConflictingPropsForWidthHeight,
type CreateIfNotExistant,
} from './adjust-css-length-command'
import type { BaseCommand, CommandFunctionResult, WhenToRun } from './commands'
import { addToastPatch } from './show-toast-command'
import { applyValuesAtPath } from './utils/property-utils'
import { getCSSNumberFromStyleInfo } from './utils/property-utils'
import type { InteractionLifecycle } from '../canvas-strategies/canvas-strategy-types'
import type { StyleUpdate } from '../plugins/style-plugins'
import { getActivePlugin, runStyleUpdateForStrategy } from '../plugins/style-plugins'

type CssNumberOrKeepOriginalUnit =
| { type: 'EXPLICIT_CSS_NUMBER'; value: CSSNumber | CSSKeyword }
Expand All @@ -45,10 +37,13 @@ export function setValueKeepingOriginalUnit(
return { type: 'KEEP_ORIGINAL_UNIT', valuePx: valuePx, parentDimensionPx: parentDimensionPx }
}

type CSSLengthProperty = LengthProperty | 'zIndex'
type CSSLengthPropertyPath = PropertyPath<['style', CSSLengthProperty]>

export interface SetCssLengthProperty extends BaseCommand {
type: 'SET_CSS_LENGTH_PROPERTY'
target: ElementPath
property: PropertyPath
property: CSSLengthPropertyPath
value: CssNumberOrKeepOriginalUnit
parentFlexDirection: FlexDirection | null
createIfNonExistant: CreateIfNotExistant
Expand All @@ -58,7 +53,7 @@ export interface SetCssLengthProperty extends BaseCommand {
export function setCssLengthProperty(
whenToRun: WhenToRun,
target: ElementPath,
property: PropertyPath,
property: CSSLengthPropertyPath,
value: CssNumberOrKeepOriginalUnit,
parentFlexDirection: FlexDirection | null,
createIfNonExistant: CreateIfNotExistant = 'create-if-not-existing', // TODO remove the default value and set it explicitly everywhere
Expand Down Expand Up @@ -90,33 +85,22 @@ export const runSetCssLengthProperty = (
command.parentFlexDirection,
)

// Identify the current value, whatever that may be.
const currentValue: GetModifiableAttributeResult = withUnderlyingTargetFromEditorState(
command.target,
editorState,
left(`no target element was found at path ${EP.toString(command.target)}`),
(_, element) => {
if (isJSXElement(element)) {
return getModifiableJSXAttributeAtPath(element.props, command.property)
} else {
return left(`No JSXElement was found at path ${EP.toString(command.target)}`)
}
},
)
if (isLeft(currentValue)) {
const styleInfo = getActivePlugin(editorStateWithPropsDeleted).styleInfoFactory({
projectContents: editorStateWithPropsDeleted.projectContents,
})(command.target)

if (styleInfo == null) {
return {
editorStatePatches: [],
commandDescription: `Set Css Length Prop: ${EP.toUid(command.target)}/${PP.toString(
command.property,
)} not applied as value is not writeable.`,
commandDescription: `Set Css Length Prop: Element not found at ${EP.toUid(command.target)}`,
}
}
const currentModifiableValue = currentValue.value
const simpleValueResult = jsxSimpleAttributeToValue(currentModifiableValue)

const targetPropertyNonExistant: boolean = currentModifiableValue.type === 'ATTRIBUTE_NOT_FOUND'
const property = command.property.propertyElements[1] // the safety of propertyElements[1] is guaranteed by the LengthPropertyPath type

const currentValue = getCSSNumberFromStyleInfo(styleInfo, property)
if (
targetPropertyNonExistant &&
currentValue.type === 'not-found' &&
command.createIfNonExistant === 'do-not-create-if-doesnt-exist'
) {
return {
Expand All @@ -127,18 +111,18 @@ export const runSetCssLengthProperty = (
}
}

let propsToUpdate: Array<ValueAtPath> = []
let propsToUpdate: Array<StyleUpdate> = []

let percentageValueWasReplaced: boolean = false
const javascriptExpressionValueWasReplaced: boolean = isLeft(simpleValueResult) // Left for jsxSimpleAttributeToValue means "not simple" which means a javascript expression like `5 + props.hello`
const javascriptExpressionValueWasReplaced = currentValue.type === 'not-css-number'

const parsePercentResult = parseCSSPercent(simpleValueResult.value)
if (
isRight(parsePercentResult) &&
currentValue.type === 'css-number' &&
currentValue.number.unit === '%' &&
command.value.type === 'KEEP_ORIGINAL_UNIT' &&
command.value.parentDimensionPx != null
) {
const currentValuePercent = parsePercentResult.value
const currentValuePercent = currentValue.number
const valueInPercent = roundTo(
(command.value.valuePx / command.value.parentDimensionPx) * 100,
2,
Expand All @@ -150,8 +134,9 @@ export const runSetCssLengthProperty = (
const newValue = printCSSNumber(newValueCssNumber, null)

propsToUpdate.push({
path: command.property,
value: jsExpressionValue(newValue, emptyComments),
type: 'set',
property: property,
value: newValue,
})
} else {
const newCssValue =
Expand All @@ -161,29 +146,31 @@ export const runSetCssLengthProperty = (

if (
command.whenReplacingPercentageValues === 'warn-about-replacement' &&
isRight(parsePercentResult)
currentValue.type === 'css-number' &&
currentValue.number.unit === '%'
) {
percentageValueWasReplaced = true
}

const printedValue = printCSSNumberOrKeyword(newCssValue, 'px')

propsToUpdate.push({
path: command.property,
value: jsExpressionValue(printedValue, emptyComments),
type: 'set',
property: property,
value: printedValue,
})
}

// Apply the update to the properties.
const { editorStatePatch: propertyUpdatePatch } = applyValuesAtPath(
const { editorStatePatches } = runStyleUpdateForStrategy(
interactionLifecycle,
editorStateWithPropsDeleted,
command.target,
propsToUpdate,
)

// Always include the property update patch, but potentially also include a warning
// that a percentage based property was replaced with a pixel based one.
let editorStatePatches: Array<EditorStatePatch> = [propertyUpdatePatch]
if (percentageValueWasReplaced) {
editorStatePatches.push(
addToastPatch(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -110,12 +110,8 @@ export type GetCSSNumberFromStyleInfoResult =

export function getCSSNumberFromStyleInfo(
styleInfo: StyleInfo,
property: string,
property: keyof StyleInfo,
): GetCSSNumberFromStyleInfoResult {
if (!isStyleInfoKey(property)) {
return { type: 'not-found' }
}

const prop = styleInfo[property]
if (prop == null || prop.type === 'not-found') {
return { type: 'not-found' }
Expand Down
2 changes: 2 additions & 0 deletions editor/src/components/canvas/plugins/inline-style-plugin.ts
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,7 @@ export const InlineStylePlugin: StylePlugin = {
const paddingBottom = getPropertyFromInstance('paddingBottom', element.props)
const paddingLeft = getPropertyFromInstance('paddingLeft', element.props)
const paddingRight = getPropertyFromInstance('paddingRight', element.props)
const zIndex = getPropertyFromInstance('zIndex', element.props)

return {
gap: gap,
Expand All @@ -89,6 +90,7 @@ export const InlineStylePlugin: StylePlugin = {
paddingBottom: paddingBottom,
paddingLeft: paddingLeft,
paddingRight: paddingRight,
zIndex: zIndex,
}
},
updateStyles: (editorState, elementPath, updates) => {
Expand Down
2 changes: 2 additions & 0 deletions editor/src/components/canvas/plugins/tailwind-style-plugin.ts
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ const TailwindPropertyMapping: Record<string, string> = {
paddingRight: 'paddingRight',
paddingBottom: 'paddingBottom',
paddingLeft: 'paddingLeft',
zIndex: 'zIndex',
}

function isSupportedTailwindProperty(prop: unknown): prop is keyof typeof TailwindPropertyMapping {
Expand Down Expand Up @@ -119,6 +120,7 @@ export const TailwindPlugin = (config: Config | null): StylePlugin => ({
mapping[TailwindPropertyMapping.paddingLeft],
cssParsers.paddingLeft,
),
zIndex: parseTailwindProperty(mapping[TailwindPropertyMapping.zIndex], cssParsers.zIndex),
}
},
updateStyles: (editorState, elementPath, updates) => {
Expand Down

0 comments on commit 17c6983

Please sign in to comment.