feature(groups) Completely disallow percentage based group children. (#…

…4302) - Change `InvalidGroupState` value `'child-has-percentage-pins-without-group-size'` to `'child-has-percentage-pins'`. - Remove a handful of checks to see if the groups have an explicit size. - Add a test that checks for a child with percentage pins but explicit dimensions.
concrete-utopia · Oct 3, 2023 · 3924b6d · 3924b6d
1 parent 44a1171
commit 3924b6d
Show file tree

Hide file tree

Showing 5 changed files with 69 additions and 47 deletions.
diff --git a/editor/src/components/canvas/canvas-strategies/strategies/group-helpers.ts b/editor/src/components/canvas/canvas-strategies/strategies/group-helpers.ts
@@ -94,7 +94,7 @@ export type GroupValidity = 'valid' | 'warning' | 'error'
 
 export type InvalidGroupState =
   | 'child-not-position-absolute'
-  | 'child-has-percentage-pins-without-group-size'
+  | 'child-has-percentage-pins'
   | 'child-has-missing-pins'
   | 'child-does-not-honour-props-position'
   | 'child-does-not-honour-props-size'
@@ -103,7 +103,7 @@ export type InvalidGroupState =
 
 export function groupValidityFromInvalidGroupState(groupState: InvalidGroupState): GroupValidity {
   switch (groupState) {
-    case 'child-has-percentage-pins-without-group-size':
+    case 'child-has-percentage-pins':
     case 'child-has-missing-pins':
     case 'group-has-percentage-pins':
       return 'warning'
@@ -139,8 +139,8 @@ export function invalidGroupStateToString(state: InvalidGroupState): string {
     // children state
     case 'child-not-position-absolute':
       return 'All children of group should have absolute position'
-    case 'child-has-percentage-pins-without-group-size':
-      return 'Group needs dimensions to use children with % pins'
+    case 'child-has-percentage-pins':
+      return 'Children of a group should not have percentage pins'
     case 'child-has-missing-pins':
       return 'All children of group should have valid pins'
     case 'child-does-not-honour-props-position':
@@ -261,7 +261,6 @@ function maybeInvalidGroupChildren(
   allElementProps: AllElementProps,
   projectContents: ProjectContentTreeRoot,
 ): InvalidGroupState | 'valid' {
-  const groupHasExplicitSize = checkGroupHasExplicitSize(group)
   const childPaths = MetadataUtils.getChildrenUnordered(metadata, path).map(
     (child) => child.elementPath,
   )
@@ -274,11 +273,7 @@ function maybeInvalidGroupChildren(
   for (const childPath of flattenedChildPaths) {
     const childMetadata = MetadataUtils.findElementByElementPath(metadata, childPath)
     if (childMetadata != null) {
-      const childGroupState = getGroupChildState(
-        projectContents,
-        childMetadata,
-        groupHasExplicitSize,
-      )
+      const childGroupState = getGroupChildState(projectContents, childMetadata)
       if (isInvalidGroupState(childGroupState)) {
         return childGroupState
       }
@@ -287,13 +282,10 @@ function maybeInvalidGroupChildren(
   return 'valid'
 }
 
-export function getGroupChildStateFromJSXElement(
-  jsxElement: JSXElement,
-  groupHasExplicitSize: boolean,
-): GroupState {
+export function getGroupChildStateFromJSXElement(jsxElement: JSXElement): GroupState {
   return (
     maybeGroupChildNotPositionAbsolutely(jsxElement) ??
-    maybeGroupChildHasPercentagePinsWithoutGroupSize(jsxElement, groupHasExplicitSize) ??
+    maybeGroupChildHasPercentagePinsWithoutGroupSize(jsxElement) ??
     maybeGroupChildHasMissingPins(jsxElement) ??
     'valid'
   )
@@ -313,7 +305,6 @@ export function getGroupValidity(
 function getGroupChildState(
   projectContents: ProjectContentTreeRoot,
   elementMetadata: ElementInstanceMetadata | null,
-  groupHasExplicitSize: boolean,
 ): GroupState {
   if (elementMetadata == null) {
     return 'unknown'
@@ -332,7 +323,7 @@ function getGroupChildState(
     return 'child-does-not-honour-props-size'
   }
 
-  return getGroupChildStateFromJSXElement(jsxElement, groupHasExplicitSize)
+  return getGroupChildStateFromJSXElement(jsxElement)
 }
 
 export function getGroupChildStateWithGroupMetadata(
@@ -345,11 +336,7 @@ export function getGroupChildStateWithGroupMetadata(
     return 'unknown'
   }
 
-  return getGroupChildState(
-    projectContents,
-    elementMetadata,
-    checkGroupHasExplicitSize(groupElement),
-  )
+  return getGroupChildState(projectContents, elementMetadata)
 }
 
 export function isMaybeGroupForWrapping(element: JSXElementChild, imports: Imports): boolean {
@@ -415,20 +402,11 @@ export function maybeGroupChildNotPositionAbsolutely(
 
 export function maybeGroupChildHasPercentagePinsWithoutGroupSize(
   jsxElement: JSXElement | null,
-  groupHasExplicitSize: boolean,
 ): InvalidGroupState | null {
   if (jsxElement == null) {
     return 'unknown'
   }
-  return !groupHasExplicitSize && elementHasPercentagePins(jsxElement)
-    ? 'child-has-percentage-pins-without-group-size'
-    : null
-}
-
-export function maybeGroupChildWithoutFixedSizeForFill(
-  group: JSXElement | null,
-): InvalidGroupState | null {
-  return !checkGroupHasExplicitSize(group) ? 'child-has-percentage-pins-without-group-size' : null
+  return elementHasPercentagePins(jsxElement) ? 'child-has-percentage-pins' : null
 }
 
 export function groupErrorToastCommand(state: InvalidGroupState): ShowToastCommand {
@@ -500,8 +478,7 @@ export function groupStateFromJSXElement(
     )
   } else if (treatElementAsGroupLike(metadata, EP.parentPath(path))) {
     // group child
-    const group = MetadataUtils.getJSXElementFromMetadata(metadata, EP.parentPath(path))
-    return getGroupChildStateFromJSXElement(element, checkGroupHasExplicitSize(group))
+    return getGroupChildStateFromJSXElement(element)
   } else {
     // not a group
     return null

diff --git a/editor/src/components/inspector/common/inspector-end-to-end-tests.spec.browser2.tsx b/editor/src/components/inspector/common/inspector-end-to-end-tests.spec.browser2.tsx
@@ -3379,7 +3379,61 @@ describe('inspector tests with real metadata', () => {
       await setControlValue('position-left-number-input', '25%', renderResult.renderedDOM)
 
       expect(getFrame(targetPath, renderResult)).toBe(elementFrame)
-      expectGroupToast(renderResult, 'child-has-percentage-pins-without-group-size')
+      expectGroupToast(renderResult, 'child-has-percentage-pins')
+    })
+    it('ignores settings percentage pins on a group child if the parent has explicit width and height', async () => {
+      const renderResult = await renderTestEditorWithCode(
+        makeTestProjectCodeWithSnippetStyledComponents(`
+          <div
+            style={{ position: 'absolute', backgroundColor: '#FFFFFF' }}
+            data-uid='aaa'
+          >
+            <Group
+              data-uid='group'
+              style={{
+                position: 'absolute',
+                left: 10,
+                top: 10,
+                width: 100,
+                height: 100,
+              }}
+            >
+              <div
+                data-uid='foo'
+                style={{
+                  position: 'absolute',
+                  top: 10,
+                  bottom: 20,
+                  left: 30,
+                  right: 40,
+                }}
+              />
+            </Group>
+          </div>
+      `),
+        'await-first-dom-report',
+      )
+
+      const targetPath = EP.appendNewElementPath(TestScenePath, ['aaa', 'group', 'foo'])
+
+      await act(async () => {
+        const dispatchDone = renderResult.getDispatchFollowUpActionsFinished()
+        await renderResult.dispatch([selectComponents([targetPath], false)], false)
+        await dispatchDone
+      })
+
+      const leftControl = (await renderResult.renderedDOM.findByTestId(
+        'position-left-number-input',
+      )) as HTMLInputElement
+
+      expect(leftControl.value).toBe('30')
+
+      const elementFrame = getFrame(targetPath, renderResult)
+
+      await setControlValue('position-left-number-input', '25%', renderResult.renderedDOM)
+
+      expect(getFrame(targetPath, renderResult)).toBe(elementFrame)
+      expectGroupToast(renderResult, 'child-has-percentage-pins')
     })
     describe('group children', () => {
       const tests: {

diff --git a/editor/src/components/inspector/common/layout-property-path-hooks.ts b/editor/src/components/inspector/common/layout-property-path-hooks.ts
@@ -46,7 +46,6 @@ import { getFramePointsFromMetadata, MaxContent } from '../inspector-common'
 import { mapDropNulls } from '../../../core/shared/array-utils'
 import {
   maybeInvalidGroupState,
-  maybeGroupChildWithoutFixedSizeForFill,
   groupErrorToastAction,
 } from '../../canvas/canvas-strategies/strategies/group-helpers'
 
@@ -416,11 +415,7 @@ export function usePinToggling(): UsePinTogglingResult {
               : null
           },
           onGroupChild: (path) => {
-            const group = MetadataUtils.getJSXElementFromMetadata(
-              jsxMetadataRef.current,
-              EP.parentPath(path),
-            )
-            return maybeGroupChildWithoutFixedSizeForFill(group) ?? null
+            return 'child-has-percentage-pins'
           },
         },
       )

diff --git a/editor/src/components/inspector/inspector-strategies/fill-container-basic-strategy.ts b/editor/src/components/inspector/inspector-strategies/fill-container-basic-strategy.ts
@@ -23,7 +23,6 @@ import {
 } from '../../canvas/commands/set-css-length-command'
 import {
   groupErrorToastCommand,
-  maybeGroupChildWithoutFixedSizeForFill,
   maybeInvalidGroupState,
 } from '../../canvas/canvas-strategies/strategies/group-helpers'
 
@@ -45,8 +44,7 @@ export const fillContainerStrategyFlow = (
     const invalidGroupState = maybeInvalidGroupState(elements, metadata, {
       onGroup: () => 'group-has-percentage-pins',
       onGroupChild: (path) => {
-        const group = MetadataUtils.getJSXElementFromMetadata(metadata, EP.parentPath(path))
-        return maybeGroupChildWithoutFixedSizeForFill(group) ?? null
+        return 'child-has-percentage-pins'
       },
     })
     if (invalidGroupState != null) {

diff --git a/editor/src/components/inspector/inspector-strategies/fixed-size-basic-strategy.ts b/editor/src/components/inspector/inspector-strategies/fixed-size-basic-strategy.ts
@@ -13,7 +13,6 @@ import type { InspectorStrategy } from './inspector-strategy'
 import { queueGroupTrueUp } from '../../canvas/commands/queue-group-true-up-command'
 import {
   groupErrorToastCommand,
-  maybeGroupChildWithoutFixedSizeForFill,
   maybeInvalidGroupState,
 } from '../../canvas/canvas-strategies/strategies/group-helpers'
 import { trueUpElementChanged } from '../../../components/editor/store/editor-state'
@@ -32,8 +31,7 @@ export const fixedSizeBasicStrategy = (
     const invalidGroupState = maybeInvalidGroupState(elementPaths, metadata, {
       onGroup: () => (value.unit === '%' ? 'group-has-percentage-pins' : null),
       onGroupChild: (path) => {
-        const group = MetadataUtils.getJSXElementFromMetadata(metadata, EP.parentPath(path))
-        return value.unit === '%' ? maybeGroupChildWithoutFixedSizeForFill(group) : null
+        return value.unit === '%' ? 'child-has-percentage-pins' : null
       },
     })
     if (invalidGroupState != null) {