Skip to content
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

Convert group child percentage pins to pixel based pins. #4314

Merged
merged 1 commit into from
Oct 4, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ import { resizeElement } from './absolute-resize.test-utils'
import { changeInspectorNumberControl } from '../../../inspector/common/inspector.test-utils'
import { ParentOutlinesTestIdSuffix } from '../../controls/parent-outlines'
import { ParentBoundsTestIdSuffix } from '../../controls/parent-bounds'
import { safeIndex } from '../../../../core/shared/array-utils'

const GroupPath = `${BakedInStoryboardUID}/${TestSceneUID}/${TestAppUID}:root-div/group`

Expand Down Expand Up @@ -2156,6 +2157,11 @@ describe('Groups behaviors', () => {
await selectComponentsForTest(editor, [fromString(GroupPath)])
await resizeElement(editor, { x: 100, y: 50 }, EdgePositionBottomRight, emptyModifiers)

const toasts = editor.getEditorState().editor.toasts
expect(toasts).toHaveLength(1)
const firstToast = safeIndex(toasts, 0)
expect(firstToast?.id).toEqual('percentage-pin-replaced')

expect(groupDiv.style.width).toBe('300px')
expect(groupDiv.style.height).toBe('300px')

Expand All @@ -2178,8 +2184,8 @@ describe('Groups behaviors', () => {
assertStylePropsSet(editor, `${GroupPath}/child-2`, {
left: 150,
top: 120,
width: '50%',
height: '60%',
width: 150,
height: 180,
right: undefined,
bottom: undefined,
})
Expand Down Expand Up @@ -2212,8 +2218,8 @@ describe('Groups behaviors', () => {
assertStylePropsSet(editor, `${GroupPath}/child-2`, {
left: 175,
top: 140,
width: '50%',
height: '60%',
width: 175,
height: 210,
right: undefined,
bottom: undefined,
})
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -265,7 +265,7 @@ describe('Keyboard Absolute Resize E2E', () => {
})
})

it('keeps trueuing up groups as directions change', async () => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

haha it's like queueing but trueueing

it('keeps trueing up groups as directions change', async () => {
const renderResult = await renderTestEditorWithCode(
formatTestProjectCode(
makeTestProjectCodeWithSnippet(`
Expand Down Expand Up @@ -349,7 +349,7 @@ describe('Keyboard Absolute Resize E2E', () => {
>
<div
style={{
height: '100%',
height: 255,
position: 'absolute',
left: 0,
top: 0,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ import type {
} from '../../editor/store/editor-state'
import { deriveState, trueUpElementChanged } from '../../editor/store/editor-state'
import { patchedCreateRemixDerivedDataMemo } from '../../editor/store/remix-derived-data'
import type { FlexDirection } from '../../inspector/common/css-utils'
import { cssPixelLength, type FlexDirection } from '../../inspector/common/css-utils'
import {
isHugFromStyleAttribute,
isHugFromStyleAttributeOrNull,
Expand All @@ -39,7 +39,11 @@ import type { CreateIfNotExistant } from './adjust-css-length-command'
import { adjustCssLengthProperties, lengthPropertyToAdjust } from './adjust-css-length-command'
import type { BaseCommand, CanvasCommand, CommandFunctionResult } from './commands'
import { foldAndApplyCommandsSimple } from './commands'
import { setCssLengthProperty, setValueKeepingOriginalUnit } from './set-css-length-command'
import {
setCssLengthProperty,
setExplicitCssValue,
setValueKeepingOriginalUnit,
} from './set-css-length-command'
import type { FrameWithAllPoints } from './utils/group-resize-utils'
import { rectangleToSixFramePoints } from './utils/group-resize-utils'
import { sixFramePointsToCanvasRectangle } from './utils/group-resize-utils'
Expand Down Expand Up @@ -99,7 +103,10 @@ export const runPushIntendedBoundsAndUpdateGroups = (
)

// TODO this is the worst editor patch in history, this should be much more fine grained, only patching the elements that changed
const editorPatch = { projectContents: { $set: editorAfterResizingAncestors.projectContents } }
const editorPatch = {
projectContents: { $set: editorAfterResizingAncestors.projectContents },
toasts: { $set: editorAfterResizingAncestors.toasts },
}

const intendedBoundsPatch =
commandLifecycle === 'mid-interaction'
Expand Down Expand Up @@ -453,33 +460,37 @@ function setElementPins(
'always',
target,
PP.create('style', 'left'),
setValueKeepingOriginalUnit(framePoints.left, parentSize.width),
setExplicitCssValue(cssPixelLength(framePoints.left)),
parentFlexDirection,
'do-not-create-if-doesnt-exist',
'warn-about-replacement',
),
setCssLengthProperty(
'always',
target,
PP.create('style', 'top'),
setValueKeepingOriginalUnit(framePoints.top, parentSize.height),
setExplicitCssValue(cssPixelLength(framePoints.top)),
parentFlexDirection,
'do-not-create-if-doesnt-exist',
'warn-about-replacement',
),
setCssLengthProperty(
'always',
target,
PP.create('style', 'right'),
setValueKeepingOriginalUnit(framePoints.right, parentSize.width),
setExplicitCssValue(cssPixelLength(framePoints.right)),
parentFlexDirection,
'do-not-create-if-doesnt-exist',
'warn-about-replacement',
),
setCssLengthProperty(
'always',
target,
PP.create('style', 'bottom'),
setValueKeepingOriginalUnit(framePoints.bottom, parentSize.height),
setExplicitCssValue(cssPixelLength(framePoints.bottom)),
parentFlexDirection,
'do-not-create-if-doesnt-exist',
'warn-about-replacement',
),
]

Expand All @@ -489,9 +500,10 @@ function setElementPins(
'always',
target,
PP.create('style', 'width'),
setValueKeepingOriginalUnit(framePoints.width, parentSize.width),
setExplicitCssValue(cssPixelLength(framePoints.width)),
parentFlexDirection,
'do-not-create-if-doesnt-exist',
'warn-about-replacement',
),
)
}
Expand All @@ -501,9 +513,10 @@ function setElementPins(
'always',
target,
PP.create('style', 'height'),
setValueKeepingOriginalUnit(framePoints.height, parentSize.height),
setExplicitCssValue(cssPixelLength(framePoints.height)),
parentFlexDirection,
'do-not-create-if-doesnt-exist',
'warn-about-replacement',
),
)
}
Expand Down
35 changes: 33 additions & 2 deletions editor/src/components/canvas/commands/set-css-length-command.ts
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import { notice } from '../../../components/common/notice'
import { isLeft, isRight, left } from '../../../core/shared/either'
import * as EP from '../../../core/shared/element-path'
import {
Expand All @@ -12,7 +13,7 @@ import {
} from '../../../core/shared/jsx-attributes'
import type { ElementPath, PropertyPath } from '../../../core/shared/project-file-types'
import * as PP from '../../../core/shared/property-path'
import type { DerivedState, EditorState } from '../../editor/store/editor-state'
import type { DerivedState, EditorState, EditorStatePatch } from '../../editor/store/editor-state'
import { deriveState, withUnderlyingTargetFromEditorState } from '../../editor/store/editor-state'
import { patchedCreateRemixDerivedDataMemo } from '../../editor/store/remix-derived-data'
import type { CSSKeyword, CSSNumber, FlexDirection } from '../../inspector/common/css-utils'
Expand All @@ -26,6 +27,7 @@ import type { CreateIfNotExistant } from './adjust-css-length-command'
import { deleteConflictingPropsForWidthHeight } from './adjust-css-length-command'
import { applyValuesAtPath } from './adjust-number-command'
import type { BaseCommand, CommandFunction, WhenToRun } from './commands'
import { addToastPatch } from './show-toast-command'

type CssNumberOrKeepOriginalUnit =
| { type: 'EXPLICIT_CSS_NUMBER'; value: CSSNumber | CSSKeyword }
Expand All @@ -49,6 +51,7 @@ export interface SetCssLengthProperty extends BaseCommand {
value: CssNumberOrKeepOriginalUnit
parentFlexDirection: FlexDirection | null
createIfNonExistant: CreateIfNotExistant
whenReplacingPercentageValues: 'do-not-warn' | 'warn-about-replacement'
}

export function setCssLengthProperty(
Expand All @@ -58,6 +61,7 @@ export function setCssLengthProperty(
value: CssNumberOrKeepOriginalUnit,
parentFlexDirection: FlexDirection | null,
createIfNonExistant: CreateIfNotExistant = 'create-if-not-existing', // TODO remove the default value and set it explicitly everywhere
whenReplacingPercentageValues: SetCssLengthProperty['whenReplacingPercentageValues'] = 'do-not-warn',
): SetCssLengthProperty {
return {
type: 'SET_CSS_LENGTH_PROPERTY',
Expand All @@ -67,6 +71,7 @@ export function setCssLengthProperty(
value: value,
parentFlexDirection: parentFlexDirection,
createIfNonExistant: createIfNonExistant,
whenReplacingPercentageValues: whenReplacingPercentageValues,
}
}

Expand Down Expand Up @@ -122,6 +127,8 @@ export const runSetCssLengthProperty: CommandFunction<SetCssLengthProperty> = (

let propsToUpdate: Array<ValueAtPath> = []

let percentageValueWasReplaced: boolean = false

const parsePercentResult = parseCSSPercent(simpleValueResult.value)
if (
isRight(parsePercentResult) &&
Expand All @@ -146,6 +153,13 @@ export const runSetCssLengthProperty: CommandFunction<SetCssLengthProperty> = (
? command.value.value
: cssPixelLength(command.value.valuePx)

if (
command.whenReplacingPercentageValues === 'warn-about-replacement' &&
isRight(parsePercentResult)
) {
percentageValueWasReplaced = true
}

const printedValue = printCSSNumberOrKeyword(newCssValue, 'px')

propsToUpdate.push({
Expand All @@ -161,8 +175,25 @@ export const runSetCssLengthProperty: CommandFunction<SetCssLengthProperty> = (
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(
editorStateWithPropsDeleted.toasts,
notice(
'One or more percentage based style properties were replaced with a pixel based one.',
'INFO',
false,
'percentage-pin-replaced',
),
),
)
}

return {
editorStatePatches: [propertyUpdatePatch],
editorStatePatches: editorStatePatches,
commandDescription: `Set Css Length Prop: ${EP.toUid(command.target)}/${PP.toString(
command.property,
)} by ${
Expand Down
13 changes: 10 additions & 3 deletions editor/src/components/canvas/commands/show-toast-command.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import type { Notice, NoticeLevel } from '../../common/notice'
import { notice } from '../../common/notice'
import type { EditorState } from '../../editor/store/editor-state'
import type { EditorState, EditorStatePatch } from '../../editor/store/editor-state'
import type { InteractionLifecycle } from '../canvas-strategies/canvas-strategy-types'
import type { CommandFunctionResult, WhenToRun } from './commands'

Expand All @@ -22,6 +22,14 @@ export function showToastCommand(
}
}

export function addToastPatch(
currentToasts: ReadonlyArray<Notice>,
noticeToAdd: Notice,
): EditorStatePatch {
const updatedToasts = [...currentToasts.filter((t) => t.id !== noticeToAdd.id), noticeToAdd]
return { toasts: { $set: updatedToasts } }
}

export function runShowToastCommand(
editorState: EditorState,
command: ShowToastCommand,
Expand All @@ -34,9 +42,8 @@ export function runShowToastCommand(
}
}

const toasts = [...editorState.toasts.filter((t) => t.id !== command.notice.id), command.notice]
return {
commandDescription: 'Show a toast',
editorStatePatches: [{ toasts: { $set: toasts } }],
editorStatePatches: [addToastPatch(editorState.toasts, command.notice)],
}
}
76 changes: 76 additions & 0 deletions editor/src/components/editor/shortcuts.spec.browser2.tsx
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import { safeIndex } from '../../core/shared/array-utils'
import { BakedInStoryboardUID, BakedInStoryboardVariableName } from '../../core/model/scene-utils'
import * as EP from '../../core/shared/element-path'
import type { CanvasRectangle } from '../../core/shared/math-utils'
Expand Down Expand Up @@ -1263,6 +1264,81 @@ describe('group selection', () => {
)
})

it('wraps selected elements with percentage dimensions in a Group', async () => {
const renderResult = await renderTestEditorWithCode(
makeTestProjectCodeWithSnippet(
`<div style={{ ...props.style }} data-uid='container'>
<div
style={{
backgroundColor: '#aaaaaa33',
position: 'absolute',
left: '5%',
top: '10%',
width: '50%',
height: '20%',
}}
data-uid='aaa'
/>
<div
style={{
backgroundColor: '#aaaaaa33',
position: 'absolute',
left: '10%',
top: '15%',
width: '70%',
height: '80%',
}}
data-uid='bbb'
/>
</div>`,
),
'await-first-dom-report',
)

await selectComponentsForTest(renderResult, [
EP.fromString(`${BakedInStoryboardUID}/${TestSceneUID}/${TestAppUID}:container/aaa`),
EP.fromString(`${BakedInStoryboardUID}/${TestSceneUID}/${TestAppUID}:container/bbb`),
])

await expectSingleUndo2Saves(renderResult, async () =>
pressKey('g', { modifiers: cmdModifier }),
)

const toasts = renderResult.getEditorState().editor.toasts
expect(toasts).toHaveLength(1)
const firstToast = safeIndex(toasts, 0)
expect(firstToast?.id).toEqual('percentage-pin-replaced')

expect(getPrintedUiJsCodeWithoutUIDs(renderResult.getEditorState())).toEqual(
makeTestProjectCodeWithSnippetWithoutUIDs(
`<div style={{ ...props.style }}>
<Group style={{ position: 'absolute', left: 20, top: 40, width: 300, height: 340 }}>
<div
style={{
backgroundColor: '#aaaaaa33',
position: 'absolute',
left: 0,
top: 0,
width: 200,
height: 80,
}}
/>
<div
style={{
backgroundColor: '#aaaaaa33',
position: 'absolute',
left: 20,
top: 20,
width: 280,
height: 320,
}}
/>
</Group>
</div>`,
),
)
})

it('if Group is not imported, it is added to the imports after the Group has been inserted', async () => {
const editor = await renderTestEditorWithCode(
`import { Scene, Storyboard } from 'utopia-api'
Expand Down