feat: added grid-1 prop to existing grid layout #903
Conversation
rkeerthient
changed the title
WalkthroughAdds an align prop to the Grid component (values: "left" | "center" | "right") and threads it through rendering so single-column grids apply alignment classes per column. Grid export now includes resolveFields that returns the align field only when columns === 1; otherwise align is omitted. Default align is "left". Many visual-editor locale files received fields.alignContent and options.one (or nested variants) translations. Grid tests were expanded, reorganized, and updated to a flattened migrate input shape. Sequence Diagram(s)sequenceDiagram
participant Editor as Visual Editor UI
participant GridExport as Grid (export)
participant Resolve as resolveFields()
participant Renderer as GridSection Renderer
participant CSS as Alignment classes
Editor->>GridExport: open/inspect Grid settings
GridExport->>Resolve: resolveFields(data.props)
Resolve-->>GridExport: fields (includes `align` if columns == 1)
Editor->>GridExport: save props (columns, align?)
GridExport->>Renderer: render({ columns, align, slots })
alt columns == 1
Renderer->>CSS: map align -> md:items-*, text-*
CSS-->>Renderer: alignment classes applied per column
else columns > 1
Renderer->>CSS: no per-column align classes
CSS-->>Renderer: default grid classes
end
Renderer-->>Editor: rendered output
Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro ⛔ Files ignored due to path filters (8)
📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the Tip 📝 Customizable high-level summaries are now available in beta!You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.
Example instruction:
Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
rkeerthient
requested review from
Fondryext,
asanehisa,
benlife5 and
jwartofsky-yext
and removed request for
asanehisa and
benlife5
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/visual-editor/src/components/layoutBlocks/Grid.tsx(7 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
packages/visual-editor/src/components/layoutBlocks/Grid.tsx (1)
packages/visual-editor/src/editor/YextField.tsx (1)
YextField(163-284)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: call_unit_test / unit_tests (18.x)
- GitHub Check: call_unit_test / unit_tests (20.x)
- GitHub Check: semgrep/ci
🔇 Additional comments (6)
packages/visual-editor/src/components/layoutBlocks/Grid.tsx (6)
12-12: LGTM!The
ThemeOptionsimport is necessary for the alignment field configuration.
22-22: LGTM!The
alignprop type definition is clear and appropriate for the feature.
50-56: Consider explicit handling of undefined align.While
defaultPropssetsalign: "left", the TypeScript type allowsundefined. The current nested ternary defaults to center alignment in the else clause, which may not match the intended default behavior if the component is used outside the Puck rendering context.Consider making the default case explicit:
columns === 1 && (align === "left" ? `md:items-start text-start` : align === "right" ? `md:items-end text-end` - : `md:items-center text-center`) + : `md:items-center text-center`) // align === "center" or undefined (falls back to center)Or make the prop non-optional in the type if it should always be provided:
- align?: "left" | "center" | "right"; + align: "left" | "center" | "right";
129-129: LGTM!The default
align: "left"is sensible and matches the type definition.
131-140: LGTM! Past review feedback addressed.The
resolveFieldsimplementation correctly returns the fullgridSectionFields(including the align field) whencolumns === 1, and hides it for multi-column grids. This addresses the concern raised in the previous review about re-adding the align field when switching back to 1 column.
90-93: ThemeOptions.ALIGNMENT correctly matches the align prop type.The
alignmentOptions(referenced asThemeOptions.ALIGNMENT) provides option values"left","center", and"right"that exactly match theGridProps.aligntype definition. The implementation is correct.
packages/visual-editor/src/components/layoutBlocks/Grid.test.tsx
Outdated
Show resolved
Hide resolved
packages/visual-editor/src/components/migrations/0024_add_one_column_to_grid.ts
Outdated
Show resolved
Hide resolved
|
Warning: Component files have been updated but no migrations have been added. See https://github.com/yext/visual-editor/blob/main/packages/visual-editor/src/components/migrations/README.md for more information. |
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/visual-editor/src/components/layoutBlocks/Grid.test.tsx (1)
471-481: Fix invalid CSS color value in test data: changergb(33, 111, -219)torgb(33, 111, 219)at line 479.The negative RGB channel value is a typo—all other BodyText test cases in this file use
rgb(33, 111, 219)with the positive value. This invalid color will be clamped or ignored by browsers, affecting test consistency and color-contrast comparisons.
🧹 Nitpick comments (1)
packages/visual-editor/src/components/layoutBlocks/Grid.test.tsx (1)
500-593: New single-grid tests give good coverage of left/right/center alignment.The three v19 single-column cases (align left/right/center) with representative HeadingText, BodyText, and CTAGroup content are a solid way to exercise the new 1-grid alignment behavior via screenshots and axe, and the use of empty extra Columns keeps slot structure consistent.
If you end up adding more alignment variants, consider parameterizing these three nearly-identical cases (e.g., looping over
[{ nameSuffix, align }]) to reduce duplication.Also applies to: 595-688, 690-783
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (19)
packages/visual-editor/src/components/testing/screenshots/Grid/[desktop] version 18 - atoms used to make a CoreInfoSection.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/Grid/[desktop] version 18 - atoms used to make a HeroSection.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/Grid/[desktop] version 19 - single grid - center align.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/Grid/[desktop] version 19 - single grid - left align.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/Grid/[desktop] version 19 - single grid - right align.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/Grid/[desktop] version 19 - various atoms.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/Grid/[mobile] atoms used to make a HeroSection.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/Grid/[mobile] version 18 - atoms used to make a CoreInfoSection.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/Grid/[mobile] version 18 - atoms used to make a HeroSection.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/Grid/[mobile] version 19 - single grid - center align.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/Grid/[mobile] version 19 - single grid - left align.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/Grid/[mobile] version 19 - single grid - right align.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/Grid/[mobile] version 19 - various atoms.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/Grid/[tablet] version 18 - atoms used to make a CoreInfoSection.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/Grid/[tablet] version 18 - atoms used to make a HeroSection.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/Grid/[tablet] version 19 - single grid - center align.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/Grid/[tablet] version 19 - single grid - left align.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/Grid/[tablet] version 19 - single grid - right align.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/Grid/[tablet] version 19 - various atoms.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**
📒 Files selected for processing (26)
packages/visual-editor/locales/cs/visual-editor.json(2 hunks)packages/visual-editor/locales/da/visual-editor.json(2 hunks)packages/visual-editor/locales/de/visual-editor.json(2 hunks)packages/visual-editor/locales/en-GB/visual-editor.json(2 hunks)packages/visual-editor/locales/en/visual-editor.json(2 hunks)packages/visual-editor/locales/es/visual-editor.json(2 hunks)packages/visual-editor/locales/et/visual-editor.json(2 hunks)packages/visual-editor/locales/fi/visual-editor.json(2 hunks)packages/visual-editor/locales/fr/visual-editor.json(2 hunks)packages/visual-editor/locales/hr/visual-editor.json(2 hunks)packages/visual-editor/locales/hu/visual-editor.json(2 hunks)packages/visual-editor/locales/it/visual-editor.json(2 hunks)packages/visual-editor/locales/ja/visual-editor.json(2 hunks)packages/visual-editor/locales/lt/visual-editor.json(2 hunks)packages/visual-editor/locales/lv/visual-editor.json(2 hunks)packages/visual-editor/locales/nb/visual-editor.json(2 hunks)packages/visual-editor/locales/nl/visual-editor.json(2 hunks)packages/visual-editor/locales/pl/visual-editor.json(2 hunks)packages/visual-editor/locales/pt/visual-editor.json(2 hunks)packages/visual-editor/locales/ro/visual-editor.json(2 hunks)packages/visual-editor/locales/sk/visual-editor.json(2 hunks)packages/visual-editor/locales/sv/visual-editor.json(2 hunks)packages/visual-editor/locales/tr/visual-editor.json(2 hunks)packages/visual-editor/locales/zh-TW/visual-editor.json(2 hunks)packages/visual-editor/locales/zh/visual-editor.json(2 hunks)packages/visual-editor/src/components/layoutBlocks/Grid.test.tsx(14 hunks)
🚧 Files skipped from review as they are similar to previous changes (20)
- packages/visual-editor/locales/zh/visual-editor.json
- packages/visual-editor/locales/sk/visual-editor.json
- packages/visual-editor/locales/sv/visual-editor.json
- packages/visual-editor/locales/pl/visual-editor.json
- packages/visual-editor/locales/fi/visual-editor.json
- packages/visual-editor/locales/da/visual-editor.json
- packages/visual-editor/locales/hr/visual-editor.json
- packages/visual-editor/locales/hu/visual-editor.json
- packages/visual-editor/locales/en/visual-editor.json
- packages/visual-editor/locales/cs/visual-editor.json
- packages/visual-editor/locales/tr/visual-editor.json
- packages/visual-editor/locales/it/visual-editor.json
- packages/visual-editor/locales/pt/visual-editor.json
- packages/visual-editor/locales/ro/visual-editor.json
- packages/visual-editor/locales/et/visual-editor.json
- packages/visual-editor/locales/de/visual-editor.json
- packages/visual-editor/locales/nb/visual-editor.json
- packages/visual-editor/locales/lt/visual-editor.json
- packages/visual-editor/locales/ja/visual-editor.json
- packages/visual-editor/locales/lv/visual-editor.json
🧰 Additional context used
🧬 Code graph analysis (1)
packages/visual-editor/src/components/layoutBlocks/Grid.test.tsx (1)
packages/visual-editor/src/components/testing/componentTests.setup.ts (1)
axe(76-80)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: call_unit_test / unit_tests (18.x)
- GitHub Check: call_unit_test / unit_tests (20.x)
- GitHub Check: semgrep/ci
🔇 Additional comments (11)
packages/visual-editor/locales/fr/visual-editor.json (2)
127-127: Correct alphabetical placement and translation foralignContentfield.The French translation "Aligner le contenu" is semantically accurate and properly positioned alphabetically within the
fieldsobject.
312-312: Correct alphabetical placement and translation foroneoption.The French translation "Un" is appropriate for the grid column option and correctly positioned within the
optionsobject.packages/visual-editor/locales/nl/visual-editor.json (2)
128-128: Translation placement and value look good.The
alignContentfield translation is correctly positioned alphabetically and appears to be an accurate Dutch translation for the new grid alignment feature.
313-313: Numeric translation key addition approved.The
onetranslation is correctly placed within the options enumeration and provides the appropriate Dutch translation for the single-column grid option.packages/visual-editor/locales/en-GB/visual-editor.json (2)
126-128: Translation additions are properly positioned and consistent.The new
alignContentkey is correctly placed within the fields section alongside similar alignment-related keys (alignCard,alignItems,alignment). The label "Align Content" is clear and appropriate for UI rendering.
311-313: Column count optiononefollows established naming convention.The new
onekey is appropriately placed within the options section and maintains consistency with existing column count options (two,threeat lines 330, 337). The label "One" accurately represents the 1-column grid variant.packages/visual-editor/locales/es/visual-editor.json (2)
127-127: Translation addition for grid alignment feature looks good.The new
alignContentkey with Spanish translation "Alinear contenido" is correctly positioned alphabetically within thefieldsobject and accurately conveys the meaning of aligning content for the Grid component's single-column layout feature.
312-312: New numeric option translation is consistent with existing patterns.The
onekey with translation "Uno" is properly placed in alphabetical order withinfields.optionsand aligns with existing translations for number selections (two,three).packages/visual-editor/locales/zh-TW/visual-editor.json (1)
125-131: New zh-TW keys for content alignment and “one” are consistent.
fields.alignContentandoptions.onefollow existing terminology and numeric option patterns in this locale; I don’t see issues with wording or structure.Also applies to: 313-313
packages/visual-editor/src/components/layoutBlocks/Grid.test.tsx (2)
1-3: Test harness wiring (imports, puckConfig, migration, a11y) looks correct.The updated imports,
puckConfig.root.renderwrapper,migrateinput shape, viewport handling, screenshot assertions, and axe checks are all coherent and keep this file aligned with the shared component test setup.Please re-run the Grid visual tests in your environment to confirm screenshots and axe checks still pass after these harness adjustments.
Also applies to: 9-29, 786-806, 817-825, 832-843, 844-851
41-57: Align prop defaults in existing v18/v19 multi-column tests look fine.Explicitly setting
align: "left"in the existing 2- and 3-column cases and updating theTextListconstants to the localized object shape keeps these scenarios stable and consistent with current data structures, even if the Grid default alignment changes later. No issues from a behavior or migration standpoint.Also applies to: 165-165, 171-186, 288-297, 310-321
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/visual-editor/src/components/layoutBlocks/Grid.test.tsx (1)
496-779: Consider reducing test duplication.The three alignment test cases share identical structure except for the
alignprop value. This creates maintenance overhead—any change to the test data must be applied in three places.Consider parameterizing the alignment value or using a test factory function:
const alignments = ["left", "right", "center"] as const; const createAlignmentTest = (align: typeof alignments[number]): ComponentTest => ({ name: `version 19 - single grid - ${align} align`, document: { address: testAddress, id: "test-id", name: "Galaxy Grill", mainPhone: "+18005551010", emails: ["sumo@yext.com"], }, props: { columns: 1, align, slots: [ { Column: [ // ... shared slot content ], }, { Column: [] }, { Column: [] }, ], }, version: 19, }); // Then add to tests array: ...alignments.map(createAlignmentTest)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
packages/visual-editor/src/components/testing/screenshots/InsightSection/[desktop] version 7 props with entity values.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**
📒 Files selected for processing (1)
packages/visual-editor/src/components/layoutBlocks/Grid.test.tsx(11 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
packages/visual-editor/src/components/layoutBlocks/Grid.test.tsx (1)
packages/visual-editor/src/components/testing/componentTests.setup.ts (1)
axe(76-80)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: call_unit_test / unit_tests (20.x)
- GitHub Check: call_unit_test / unit_tests (18.x)
- GitHub Check: semgrep/ci
🔇 Additional comments (4)
packages/visual-editor/src/components/layoutBlocks/Grid.test.tsx (4)
1-29: LGTM!The import updates correctly reflect the migration to the new testing infrastructure using
@measured/puckand@vitest/browser.
496-779: Verify version number for new alignment tests.Based on past review comments, version 24 was mentioned as the newest version. The new alignment test cases use version 19. Please confirm whether:
- Version 19 is intentional for testing migration paths, or
- These tests should use version 24 to test the latest behavior
Based on past review comments.
799-799: LGTM!The simplified root render function and flattened migration data structure correctly align with the updated test infrastructure and migration patterns.
Also applies to: 815-816
845-846: LGTM!Renaming the second accessibility check result to
results2improves clarity and makes it easier to distinguish between the initial and post-interaction accessibility validations.
packages/visual-editor/src/components/layoutBlocks/Grid.test.tsx
Outdated
Show resolved
Hide resolved
...nts/testing/screenshots/Grid/[desktop] version 18 - atoms used to make a CoreInfoSection.png
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/visual-editor/src/components/layoutBlocks/Grid.test.tsx (2)
1-29: Imports and Puck config look good; minor nit onReact.ReactNodetypingThe switch to
Config/Renderand the updated imports all look consistent with the public Grid API and your test setup. One small nit:render: ({ children }: { children: React.ReactNode }) => …relies onReactbeing in scope as a type; if React isn’t globally typed in this repo, this will cause a TS error.You can either:
- Drop the explicit type and let it be inferred, or
- Import the type (e.g.
import type { ReactNode } from "react";and usechildren: ReactNode).Also applies to: 593-610
496-588: New v19 single‑grid test is a solid baseline; consider explicit align coverageThe new v19 single‑column grid test (HeadingText + BodyText + CTAGroup) is a good addition and directly exercises the 1‑column Grid path with realistic content and three slots (one populated, two empty).
Given this PR introduces an
alignprop that only applies whencolumns === 1, consider adding (here or in a nearby test) explicit test cases for non‑default align values ("center"/"right") to ensure the layout and migrations behave correctly when alignment is changed.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
packages/visual-editor/src/components/testing/screenshots/Grid/[desktop] version 19 - single grid.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/Grid/[mobile] version 19 - single grid.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/Grid/[tablet] version 19 - single grid.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**
📒 Files selected for processing (1)
packages/visual-editor/src/components/layoutBlocks/Grid.test.tsx(10 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
packages/visual-editor/src/components/layoutBlocks/Grid.test.tsx (1)
packages/visual-editor/src/components/testing/componentTests.setup.ts (1)
axe(76-80)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: call_unit_test / unit_tests (20.x)
- GitHub Check: call_unit_test / unit_tests (18.x)
- GitHub Check: semgrep/ci
🔇 Additional comments (3)
packages/visual-editor/src/components/layoutBlocks/Grid.test.tsx (3)
41-305: Multi‑column test fixtures (v18 Hero/CoreInfo) are consistent with Grid usageThe updated fixtures for the v18 Hero and CoreInfo variants look structurally sound:
columnsvalues match the intended layouts.- Adding an explicit empty
{ Column: [] }for the unused slot aligns with a 3‑slot grid model and should help keep tests closer to real content shapes.- The expanded
TextListconstant values (Delivery, Catering, Carry Out, Dine In) are valid and improve coverage of list rendering.No issues from a correctness perspective.
308-495: v19 “various atoms” fixture matches component expectationsThe v19 “various atoms” test case looks coherent with the other grid scenarios:
- Data shapes for
Address,GetDirections,Phone,Emails,BodyText, andHeadingTextmatch the patterns used elsewhere.- The added empty
{ Column: [] }maintains a consistent three‑slot structure even for a two‑column grid, which should mirror real editor data.- Inline HTML for
BodyTextuses valid color values and should play nicely with youraxecolor-contrastrule configuration.This fixture should be a good regression guard for the updated Grid behavior.
612-658: Migration + rendering harness changes look correct and add useful checksThe updated harness:
- Uses
migratewith{ root: { props: { version } }, content: [{ type: "Grid", props }] }, which cleanly threads versioning into the root.- Sets the viewport via
page.viewport(width, height)before screenshot assertions.- Waits for all
<img>elements to reportcomplete === truebefore capturing screenshots, reducing flakiness.- Runs
axeboth before and afterinteractions, which is a nice improvement for catching regressions introduced by interactions.This structure looks sound and aligns with your component testing setup.
...editor/src/components/testing/screenshots/Grid/[mobile] atoms used to make a HeroSection.png
Show resolved
Hide resolved
The existing code works because `lg:grid-cols-3` and `lg:grid-cols-2` are used in other components, but this is more correct + safer for the future Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
benlife5
left a comment
benlife5
left a comment
There was a problem hiding this comment.
Checks are going to fail/stall out due to GitHub incident. LGTM
3 participants
To address full width contents, we'll have an additional prop specifically for the 1-grid layout. When enabled, users will be able to choose the
content alignment. For 2 or 3 grid layouts, the alignment option will hidden. Here are the screencasts of the same.Grid.mov