fix: shared styles lost when new card is added #938
Conversation
|
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. |
WalkthroughThe PR introduces a shared card-styling flow across multiple page sections. A new exported helper gatherSlotStyles extracts styles from CardSlot props; wrapper components (Event/Insight/Product/Team/Testimonial) call it to derive sharedCardProps and pass backgroundColor and sharedSlotStyles into default Sequence Diagram(s)sequenceDiagram
autonumber
participant Wrapper as SectionCardsWrapper
participant Hook as useGetCardSlots.gatherSlotStyles
participant Factory as default<X>CardSlotData
participant Slots as CardSlot[] (data.props.slots)
Wrapper->>Slots: read CardSlot (first element)
Wrapper->>Hook: gatherSlotStyles(first CardSlot.props)
Hook-->>Wrapper: slotStyles map
Wrapper->>Factory: create new card (id,index, backgroundColor, slotStyles)
Factory-->>Wrapper: cardData with applied sharedSlotStyles
Wrapper->>Slots: insert/replace cardData into CardSlot array
note right of Slots: Result: generated cards inherit shared styles\nsourced from first CardSlot
Possibly related PRs
Suggested labels
Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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 |
![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
🧹 Nitpick comments (5)
packages/visual-editor/src/components/pageSections/InsightSection/InsightCardsWrapper.tsx (1)
65-74: Potential null access onprops.styles.If
CardSlot[0].props.stylesis undefined, accessing.backgroundColorat line 70 will throw. Consider adding optional chaining for defensive access:const sharedCardProps = data.props.slots.CardSlot.length === 0 ? undefined : { backgroundColor: - data.props.slots.CardSlot[0].props.styles.backgroundColor, + data.props.slots.CardSlot[0].props.styles?.backgroundColor, slotStyles: gatherSlotStyles( data.props.slots.CardSlot[0].props.slots ), };packages/visual-editor/src/components/pageSections/EventSection/EventCardsWrapper.tsx (1)
59-70: Same optional chaining improvement as InsightCardsWrapper.For consistency and defensive coding, consider adding optional chaining on
props.styles:const sharedCardProps = data.props.slots.CardSlot.length === 0 ? undefined : { backgroundColor: - data.props.slots.CardSlot[0].props.styles.backgroundColor, + data.props.slots.CardSlot[0].props.styles?.backgroundColor, truncateDescription: data.props.slots.CardSlot[0].props.truncateDescription, slotStyles: gatherSlotStyles( data.props.slots.CardSlot[0].props.slots ), };packages/visual-editor/src/components/pageSections/TeamSection/TeamCardsWrapper.tsx (1)
62-71: Consider defensive access forstylesproperty.If
CardSlot[0].props.stylesis undefined, accessing.backgroundColorwill throw. While TypeScript types may guarantee this structure, a defensive check would be safer.const sharedCardProps = data.props.slots.CardSlot.length === 0 ? undefined : { backgroundColor: - data.props.slots.CardSlot[0].props.styles.backgroundColor, + data.props.slots.CardSlot[0].props.styles?.backgroundColor, slotStyles: gatherSlotStyles( data.props.slots.CardSlot[0].props.slots ), };packages/visual-editor/src/components/pageSections/ProductSection/ProductCardsWrapper.tsx (1)
65-74: Same defensive access suggestion applies here.As noted in
TeamCardsWrapper.tsx, consider usingdata.props.slots.CardSlot[0].props.styles?.backgroundColorfor safety.packages/visual-editor/src/components/pageSections/TestimonialSection/TestimonialCardsWrapper.tsx (1)
67-76: Consistent implementation across wrappers.Same pattern as other wrappers. Consider
styles?.backgroundColorfor defensive access.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
packages/visual-editor/src/components/directory/DirectoryWrapper.tsx(1 hunks)packages/visual-editor/src/components/pageSections/EventSection/EventCard.tsx(1 hunks)packages/visual-editor/src/components/pageSections/EventSection/EventCardsWrapper.tsx(4 hunks)packages/visual-editor/src/components/pageSections/InsightSection/InsightCard.tsx(2 hunks)packages/visual-editor/src/components/pageSections/InsightSection/InsightCardsWrapper.tsx(4 hunks)packages/visual-editor/src/components/pageSections/ProductSection/ProductCard.tsx(2 hunks)packages/visual-editor/src/components/pageSections/ProductSection/ProductCardsWrapper.tsx(4 hunks)packages/visual-editor/src/components/pageSections/TeamSection/TeamCard.tsx(1 hunks)packages/visual-editor/src/components/pageSections/TeamSection/TeamCardsWrapper.tsx(4 hunks)packages/visual-editor/src/components/pageSections/TestimonialSection/TestimonialCard.tsx(1 hunks)packages/visual-editor/src/components/pageSections/TestimonialSection/TestimonialCardsWrapper.tsx(4 hunks)packages/visual-editor/src/hooks/useGetCardSlots.tsx(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (5)
packages/visual-editor/src/components/pageSections/ProductSection/ProductCardsWrapper.tsx (2)
packages/visual-editor/src/hooks/useGetCardSlots.tsx (1)
gatherSlotStyles(35-42)packages/visual-editor/src/components/pageSections/ProductSection/ProductCard.tsx (1)
defaultProductCardSlotData(54-180)
packages/visual-editor/src/components/pageSections/TeamSection/TeamCardsWrapper.tsx (2)
packages/visual-editor/src/hooks/useGetCardSlots.tsx (1)
gatherSlotStyles(35-42)packages/visual-editor/src/components/pageSections/TeamSection/TeamCard.tsx (1)
defaultTeamCardSlotData(51-208)
packages/visual-editor/src/components/pageSections/InsightSection/InsightCardsWrapper.tsx (2)
packages/visual-editor/src/hooks/useGetCardSlots.tsx (1)
gatherSlotStyles(35-42)packages/visual-editor/src/components/pageSections/InsightSection/InsightCard.tsx (1)
defaultInsightCardSlotData(50-208)
packages/visual-editor/src/components/pageSections/TeamSection/TeamCard.tsx (1)
packages/visual-editor/src/components/contentBlocks/index.ts (1)
PhoneListProps(13-13)
packages/visual-editor/src/components/pageSections/EventSection/EventCard.tsx (1)
packages/visual-editor/src/components/contentBlocks/index.ts (1)
TimestampProps(15-15)
⏰ 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 (17)
packages/visual-editor/src/components/pageSections/InsightSection/InsightCardsWrapper.tsx (1)
99-104: Correctly propagates shared styles to new cards.The implementation properly passes
backgroundColorandslotStylesfrom existing cards to newly created placeholder cards, which addresses the root cause described in the PR objectives.packages/visual-editor/src/components/pageSections/TestimonialSection/TestimonialCard.tsx (1)
39-126: Implementation correctly applies shared styles to new cards.The function signature extension and the post-processing loop at lines 119-123 properly inherit styles from existing cards, fixing the shared styles reset issue.
packages/visual-editor/src/components/pageSections/InsightSection/InsightCard.tsx (2)
50-55: Function signature correctly extended for shared style propagation.The additional
backgroundColorandsharedSlotStylesparameters enable inheriting styles from existing cards, which is the core fix for the shared styles reset issue.
201-207: Post-processing correctly applies shared slot styles.The iteration over
cardData.props.slotsand conditional style assignment ensures new cards inherit the styling configuration from existing cards.packages/visual-editor/src/components/pageSections/EventSection/EventCardsWrapper.tsx (1)
98-104: Shared style propagation correctly implemented.The implementation properly passes all shared props (
backgroundColor,truncateDescription,slotStyles) to newly created cards, maintaining consistency with the fix pattern.packages/visual-editor/src/components/pageSections/TeamSection/TeamCardsWrapper.tsx (1)
100-105: Shared styles propagation looks correct.Both code paths (entity values and static values) correctly pass
sharedCardProps?.backgroundColorandsharedCardProps?.slotStylestodefaultTeamCardSlotData, ensuring new cards inherit styling from existing cards.Also applies to: 159-164
packages/visual-editor/src/components/pageSections/ProductSection/ProductCard.tsx (2)
54-59: Function signature extension looks appropriate.The new optional parameters
backgroundColorandsharedSlotStylesenable style inheritance without breaking existing call sites that don't pass these arguments.
173-177: Style application replaces rather than merges.This completely overwrites
slotArray[0].props.styleswithsharedSlotStyles[slotKey]. This is likely intentional for full style synchronization, but note that any slot-specific defaults defined above (e.g.,aspectRatio: 1.78on ImageSlot) will be lost ifsharedSlotStylescontains that key.packages/visual-editor/src/components/pageSections/ProductSection/ProductCardsWrapper.tsx (1)
102-107: Implementation is consistent with the pattern.Both entity-driven and static value paths correctly pass shared styling to new cards, matching the fix described in the PR.
Also applies to: 161-166
packages/visual-editor/src/components/pageSections/TestimonialSection/TestimonialCardsWrapper.tsx (1)
105-110: LGTM!Both code paths correctly propagate shared styles to newly created testimonial cards.
Also applies to: 165-170
packages/visual-editor/src/components/pageSections/EventSection/EventCard.tsx (3)
55-61: EventCard includes additionaltruncateDescriptionparameter.Unlike other card factories, this one includes a card-specific style property (
truncateDescription). This is appropriate given EventCard's unique truncation feature.
62-70: Defaults are appropriate.Using nullish coalescing for
backgroundColorandtruncateDescription(defaulting totrue) maintains backward compatibility while enabling style inheritance.
187-191: Same style replacement behavior as other cards.This completely replaces slot styles when
sharedSlotStyles[slotKey]is present. This is consistent withProductCard.tsxand other card implementations.packages/visual-editor/src/components/pageSections/TeamSection/TeamCard.tsx (3)
51-56: Function signature correctly supports shared styling propagation.The new optional parameters
backgroundColorandsharedSlotStylesallow newly created cards to inherit styling from existing cards, addressing the root cause described in the PR objectives.
57-199: Comprehensive card data construction with proper type annotations.The factory now builds a fully structured
cardDataobject with all slot definitions, usingsatisfiesannotations for type safety. The conditional spreading ofidandindexproperties and the fallback forbackgroundColorare well-implemented.
201-205: This suggestion is not applicable. The current implementation is correct.gatherSlotStylesextracts complete style objects from an existing card (value[0].props.styles || {}), not partial styles. WhensharedSlotStylesis applied to new cards, direct assignment is the intended behavior—it replaces the default styles with styles from the first card. UsingdeepMergeor spread operators would prevent style updates from taking effect properly and is unnecessary here.Likely an incorrect or invalid review comment.
packages/visual-editor/src/components/directory/DirectoryWrapper.tsx (1)
146-147: No changes needed—the implementation correctly applies shared card styles and handles undefined gracefully.The code uses
CardSlot[0]'s styles and slots for all newly created cards, which is the correct approach for the shared styling feature. ThedefaultDirectoryCardSlotDatafactory function safely handlesundefinedvalues through optional chaining and null coalescing operators (e.g.,existingCardStyle?.backgroundColor ?? backgroundColors.background1.value), ensuring defaults are applied when styles are not provided. WhenCardSlotis empty,undefinedis passed and the factory correctly falls back to default values.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/visual-editor/src/hooks/useGetCardSlots.tsx (1)
35-42: Good defensive access with optional chaining.The optional chaining (
value?.[0]?.props?.styles) correctly addresses the concern from the previous review about potential runtime errors when slot arrays are empty. The fallback to{}ensures consistent behavior.Consider adding an explicit return type annotation.
For improved type safety and documentation, consider adding a return type:
-export const gatherSlotStyles = (slotProps: Record<string, Slot>) => { +export const gatherSlotStyles = (slotProps: Record<string, Slot>): Record<string, any> => { return Object.fromEntries( Object.entries(slotProps).map(([key, value]) => [ key, value?.[0]?.props?.styles || {}, ]) ); };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/visual-editor/src/hooks/useGetCardSlots.tsx(1 hunks)
⏰ 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 (1)
packages/visual-editor/src/hooks/useGetCardSlots.tsx (1)
29-29: LGTM! Clean refactoring for code reuse.Extracting the slot styles logic into
gatherSlotStylesimproves maintainability and aligns well with the PR objective of sharing card styling across multiple wrapper components.
benlife5
requested review from
asanehisa,
briantstephan,
colton-demetriou and
mkilpatrick
asanehisa
left a comment
asanehisa
left a comment
There was a problem hiding this comment.
lgtm, was FAQ / adding a "card" to faq ok?
|
Interestingly, the styles aren't shared at all between each item in FAQ Section... will add a note to the FAQ revamp item |
3 participants
When a new card was added to a card section (either via adding in the constant value or via a new list item on the entity), the shared card styles were being reset to the defaults. This was because the new card was being created with default props, triggering it to update the shared context, triggering the existing cards to dispatch updates. This change makes it so that new cards are initialized with the shared styling props of the already existing cards.