fix[gen2-sdks]: accordion visual editing and order (#3717)

## Description for visual editing, we needed to pass the correct path. In gen1, if `component.options` is not already prefixed we add them to the path: https://github.com/sidmohanty11/builder/blob/fix/accordion-gen2/packages/react/src/components/builder-blocks.component.tsx#L48 so passing just `item.$index.detail` didn't work for misordering of item details, we should only use `order` when `props.grid === true`: https://github.com/sidmohanty11/builder/blob/fix/accordion-gen2/packages/widgets/src/components/Accordion.tsx/#L75-L80 Jira https://builder-io.atlassian.net/browse/ENG-7330 _Screenshot_ If relevant, add a screenshot or two of the changes you made.
BuilderIO · Nov 5, 2024 · efa4798 · efa4798
1 parent c3b0f33
commit efa4798
Show file tree

Hide file tree

Showing 10 changed files with 160 additions and 24 deletions.
diff --git a/.changeset/breezy-dolphins-drive.md b/.changeset/breezy-dolphins-drive.md
@@ -0,0 +1,11 @@
+---
+'@builder.io/sdk-angular': patch
+'@builder.io/sdk-qwik': patch
+'@builder.io/sdk-react': patch
+'@builder.io/sdk-react-native': patch
+'@builder.io/sdk-solid': patch
+'@builder.io/sdk-svelte': patch
+'@builder.io/sdk-vue': patch
+---
+
+Fix: accordion block order of items and visual editing empty blocks
diff --git a/packages/react-tests/react-vite/src/App.tsx b/packages/react-tests/react-vite/src/App.tsx
@@ -48,7 +48,7 @@ function App() {
     }
   }, []);
 
-  return props || PAGES[window.location.pathname]?.isVisualEditingTest ? (
+  return props || PAGES[window.location.pathname]?.isGen1VisualEditingTest ? (
     <BuilderComponent {...props} />
   ) : (
     <div>Content Not Found</div>

diff --git a/packages/sdks-tests/src/e2e-tests/accordion.spec.ts b/packages/sdks-tests/src/e2e-tests/accordion.spec.ts
@@ -17,7 +17,10 @@ test.describe('Accordion', () => {
       await page.locator(`text=Item ${i}`).click({ timeout: 10000 });
     }
   });
-  test('Accordion opens', async ({ page, sdk }) => {
+  test('Accordion opens and item details are visible in the correct order', async ({
+    page,
+    sdk,
+  }) => {
     test.fail(
       excludeTestFor(
         {
@@ -29,8 +32,31 @@ test.describe('Accordion', () => {
     await page.goto('/accordion');
 
     for (let i = 1; i <= 3; i++) {
-      await page.locator(`text=Item ${i}`).click({ timeout: 10000 });
-      await expect(page.locator(`text=Inside Item ${i}`)).toBeVisible();
+      const itemTitle = page.getByText(`Item ${i}`, { exact: true });
+      await itemTitle.click({ timeout: 10000 });
+
+      const itemDetail = page.getByText(`Inside Item ${i}`, { exact: true });
+      await expect(itemDetail).toBeVisible();
+
+      const titleBox = await itemTitle.boundingBox();
+      const detailBox = await itemDetail.boundingBox();
+
+      expect(detailBox).toBeDefined();
+      expect(titleBox).toBeDefined();
+
+      if (titleBox && detailBox) {
+        expect(detailBox.y).toBeGreaterThan(titleBox.y + titleBox.height);
+
+        if (i < 3) {
+          const nextItemTitle = page.getByText(`Item ${i + 1}`, { exact: true });
+          const nextTitleBox = await nextItemTitle.boundingBox();
+          expect(nextTitleBox).toBeDefined();
+
+          if (nextTitleBox) {
+            expect(detailBox.y + detailBox.height).toBeLessThan(nextTitleBox.y);
+          }
+        }
+      }
     }
   });
   test('Content is hidden when accordion is closed', async ({ page, sdk }) => {

diff --git a/packages/sdks-tests/src/e2e-tests/editing.spec.ts b/packages/sdks-tests/src/e2e-tests/editing.spec.ts
@@ -16,6 +16,7 @@ import {
 import { MODIFIED_EDITING_COLUMNS } from '../specs/editing-columns-inner-layout.js';
 import { ADD_A_TEXT_BLOCK } from '../specs/duplicated-content-using-nested-symbols.js';
 import { EDITING_STYLES } from '../specs/editing-styles.js';
+import { ACCORDION_WITH_NO_DETAIL } from '../specs/accordion.js';
 
 const editorTests = ({ noTrustedHosts }: { noTrustedHosts: boolean }) => {
   test('correctly updates Text block', async ({ page, basePort, packageName, sdk }) => {
@@ -232,6 +233,68 @@ test.describe('Visual Editing', () => {
     });
   });
 
+  test.describe('Accordion block', () => {
+    test('inserting a new detail item adds it to the correct place in the accordion', async ({
+      page,
+      sdk,
+      basePort,
+      packageName,
+    }) => {
+      test.skip(
+        packageName === 'nextjs-sdk-next-app' ||
+          packageName === 'gen1-next' ||
+          packageName === 'gen1-react' ||
+          packageName === 'gen1-remix'
+      );
+      await launchEmbedderAndWaitForSdk({ path: '/accordion-no-detail', basePort, page, sdk });
+
+      const item1 = page.frameLocator('iframe').getByText('Item 1');
+      const NEW_DETAILS_TEXT = 'new detail';
+
+      await item1.click();
+
+      await expect(page.frameLocator('iframe').getByText(NEW_DETAILS_TEXT)).not.toBeVisible();
+
+      // reset
+      await item1.click();
+
+      const accordion = cloneContent(ACCORDION_WITH_NO_DETAIL);
+
+      // insert new detail item
+      accordion.data.blocks[0].component.options.items[0].detail = [
+        {
+          id: 'some-random-id',
+          component: {
+            name: 'Text',
+            options: { text: NEW_DETAILS_TEXT },
+          },
+        },
+      ];
+
+      await sendContentUpdateMessage({
+        page,
+        newContent: accordion,
+        model: 'page',
+      });
+
+      await item1.click();
+
+      const detailElement = page.frameLocator('iframe').getByText(NEW_DETAILS_TEXT);
+      await detailElement.waitFor();
+
+      const [titleBox, detailBox] = await Promise.all([
+        item1.boundingBox(),
+        detailElement.boundingBox(),
+      ]);
+
+      if (!titleBox || !detailBox) {
+        throw new Error('Title or detail box not found');
+      }
+
+      expect(detailBox.y).toBeGreaterThan(titleBox.y);
+    });
+  });
+
   test.describe('fails for empty trusted hosts', () => {
     test.fail();
     editorTests({ noTrustedHosts: true });

diff --git a/packages/sdks-tests/src/specs/accordion.ts b/packages/sdks-tests/src/specs/accordion.ts
@@ -318,3 +318,20 @@ export const ACCORDION_GRID = {
     })),
   },
 };
+
+export const ACCORDION_WITH_NO_DETAIL = {
+  ...ACCORDION,
+  data: {
+    ...ACCORDION.data,
+    blocks: ACCORDION.data.blocks.map(block => ({
+      ...block,
+      component: {
+        ...block.component,
+        options: {
+          ...block.component.options,
+          items: block.component.options.items.map(item => ({ ...item, detail: [] })),
+        },
+      },
+    })),
+  },
+};
diff --git a/packages/sdks-tests/src/specs/index.ts b/packages/sdks-tests/src/specs/index.ts
@@ -52,7 +52,12 @@ import type { BuilderContent } from './types.js';
 import { CONTENT as video } from './video.js';
 import { CUSTOM_COMPONENTS } from './custom-components.js';
 import { BASIC_STYLES } from './basic-styles.js';
-import { ACCORDION, ACCORDION_GRID, ACCORDION_ONE_AT_A_TIME } from './accordion.js';
+import {
+  ACCORDION,
+  ACCORDION_GRID,
+  ACCORDION_ONE_AT_A_TIME,
+  ACCORDION_WITH_NO_DETAIL,
+} from './accordion.js';
 import { SYMBOL_TRACKING } from './symbol-tracking.js';
 import { COLUMNS_WITH_DIFFERENT_WIDTHS } from './columns-with-different-widths.js';
 import { CUSTOM_COMPONENTS_MODELS_RESTRICTION } from './custom-components-models.js';
@@ -160,6 +165,7 @@ export const PAGES: Record<string, Page> = {
   '/accordion': { content: ACCORDION },
   '/accordion-one-at-a-time': { content: ACCORDION_ONE_AT_A_TIME },
   '/accordion-grid': { content: ACCORDION_GRID },
+  '/accordion-no-detail': { content: ACCORDION_WITH_NO_DETAIL },
   '/symbol-tracking': { content: SYMBOL_TRACKING },
   '/columns-with-different-widths': { content: COLUMNS_WITH_DIFFERENT_WIDTHS },
   '/custom-components-models-show': { content: CUSTOM_COMPONENTS_MODELS_RESTRICTION },

diff --git a/packages/sdks/src/blocks/accordion/accordion.lite.tsx b/packages/sdks/src/blocks/accordion/accordion.lite.tsx
@@ -177,16 +177,18 @@ export default function Accordion(props: AccordionProps) {
               class={state.getAccordionTitleClassName(index)}
               style={{
                 ...state.accordionTitleStyles,
-                width: props.grid ? props.gridRowWidth : undefined,
-                ...(useTarget({
-                  reactNative: {},
-                  default: {
-                    order:
-                      state.openGridItemOrder !== null
-                        ? convertOrderNumberToString(index)
-                        : convertOrderNumberToString(index + 1),
-                  },
-                }) as any),
+                ...(props.grid && {
+                  width: props.gridRowWidth,
+                  ...(useTarget({
+                    reactNative: {},
+                    default: {
+                      order:
+                        state.openGridItemOrder !== null
+                          ? convertOrderNumberToString(index)
+                          : convertOrderNumberToString(index + 1),
+                    },
+                  }) as any),
+                }),
               }}
               data-index={index}
               onClick={() => state.onClick(index)}

diff --git a/packages/sdks/src/blocks/columns/columns.lite.tsx b/packages/sdks/src/blocks/columns/columns.lite.tsx
@@ -262,7 +262,7 @@ export default function Columns(props: ColumnProps) {
                 qwik: deoptSignal(column.blocks),
                 default: column.blocks,
               })}
-              path={`component.options.columns.${index}.blocks`}
+              path={`columns.${index}.blocks`}
               parent={props.builderBlock.id}
               styleProp={{
                 flexGrow: useTarget<string | number>({

diff --git a/packages/sdks/src/blocks/tabs/tabs.lite.tsx b/packages/sdks/src/blocks/tabs/tabs.lite.tsx
@@ -47,7 +47,7 @@ export default function Tabs(props: TabsProps) {
             >
               <Blocks
                 parent={props.builderBlock.id}
-                path={`component.options.tabs.${index}.label`}
+                path={`tabs.${index}.label`}
                 blocks={tab.label}
                 context={props.builderContext}
                 registeredComponents={props.builderComponents}
@@ -62,7 +62,7 @@ export default function Tabs(props: TabsProps) {
         <div>
           <Blocks
             parent={props.builderBlock.id}
-            path={`component.options.tabs.${state.activeTab}.content`}
+            path={`tabs.${state.activeTab}.content`}
             blocks={state.activeTabContent(state.activeTab)}
             context={props.builderContext}
             registeredComponents={props.builderComponents}

diff --git a/packages/sdks/src/components/blocks/blocks-wrapper.lite.tsx b/packages/sdks/src/components/blocks/blocks-wrapper.lite.tsx
@@ -46,14 +46,23 @@ export default function BlocksWrapper(props: BlocksWrapperProps) {
         .filter(Boolean)
         .join(' ');
     },
+    get dataPath() {
+      if (!props.path) {
+        return undefined;
+      }
+      const pathPrefix = 'component.options.';
+      return props.path.startsWith(pathPrefix)
+        ? props.path
+        : `${pathPrefix}${props.path || ''}`;
+    },
     onClick() {
       if (isEditing() && !props.blocks?.length) {
         window.parent?.postMessage(
           {
             type: 'builder.clickEmptyBlocks',
             data: {
               parentElementId: props.parent,
-              dataPath: props.path,
+              dataPath: state.dataPath,
             },
           },
           '*'
@@ -67,7 +76,7 @@ export default function BlocksWrapper(props: BlocksWrapperProps) {
             type: 'builder.hoverEmptyBlocks',
             data: {
               parentElementId: props.parent,
-              dataPath: props.path,
+              dataPath: state.dataPath,
             },
           },
           '*'
@@ -84,10 +93,12 @@ export default function BlocksWrapper(props: BlocksWrapperProps) {
            * React Native strips off custom HTML attributes, so we have to manually set them here
            * to ensure that blocks are correctly dropped into the correct parent.
            */
-          props.path &&
-            blocksWrapperRef.setAttribute('builder-path', props.path);
-          props.parent &&
+          if (state.dataPath) {
+            blocksWrapperRef.setAttribute('builder-path', state.dataPath);
+          }
+          if (props.parent) {
             blocksWrapperRef.setAttribute('builder-parent-id', props.parent);
+          }
         }
       },
       default: () => {},
@@ -98,7 +109,7 @@ export default function BlocksWrapper(props: BlocksWrapperProps) {
     <props.BlocksWrapper
       ref={blocksWrapperRef}
       class={state.className}
-      builder-path={props.path}
+      builder-path={state.dataPath}
       builder-parent-id={props.parent}
       {...useTarget({
         reactNative: { dataSet: { class: state.className } },