-
Notifications
You must be signed in to change notification settings - Fork 6
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
feat: add hero and banner section from builder.io #1761
base: main
Are you sure you want to change the base?
Conversation
…ching - Added new constants and utility functions for managing Builder section models. - Implemented dynamic content rendering for Hero and Banner sections using Builder. - Refactored Navbar and Footer components to support dynamic content from Builder. - Introduced new HeroCTA and HeroImageWrapper components for improved user engagement. - Updated layout and page components to fetch and render Builder content dynamically. - Enhanced NavBarGlobalBanner to accept customizable text for better localization. - Registered new components with Builder for seamless integration and customization.
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
- Exported titleVariantsConfig and pageTitleVariants for better accessibility. - Introduced AsVariantsConfig for flexible heading element selection. - Created a new PageTitle builder component to support dynamic rendering in Builder. - Registered the new PageTitle component in the Builder integration for seamless usage.
src/components/app/heroImageWrapper/heroImageWrapper.builder.tsx
Outdated
Show resolved
Hide resolved
src/components/app/heroImageWrapper/heroImageWrapper.builder.tsx
Outdated
Show resolved
Hide resolved
src/components/app/heroImageWrapper/heroImageWrapper.builder.tsx
Outdated
Show resolved
Hide resolved
src/components/app/heroImageWrapper/heroImageWrapper.builder.tsx
Outdated
Show resolved
Hide resolved
…components in Builder integration
…ntegration - Removed the 'withChildren' wrapper from both components to streamline their usage. - Updated the component registration to enhance attribute handling and improve key management. - Added 'friendlyName' and 'noWrap' properties for better integration with Builder's UI.
…eTitle components in Builder integration - Simplified the component registration for both PageSubTitle and PageTitle by eliminating the 'canHaveChildren' property, enhancing clarity and usability in the Builder environment.
…n HeroCTA component - Updated HeroCTAProps interface to allow optional properties for authenticated and unauthenticated props, enhancing flexibility in component usage.
…ntegration - Removed unnecessary properties ('as' and 'withoutBalancer') from the Builder component props, streamlining the interface. - Updated input configuration to require a 'text' property, enhancing clarity and usability in the Builder environment. - Improved overall component registration for better integration with Builder's UI.
…onents in Builder integration - Eliminated the defaultChildren property from both components to simplify their configuration and enhance usability in the Builder environment.
userAttributes: { | ||
urlPath: pathname, | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't we use prerender: false
here?
userAttributes: { | ||
urlPath: pathname, | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't we use prerender: false
here?
HERO = 'hero-model', | ||
BANNER = 'banner', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
HERO = 'hero-model', | |
BANNER = 'banner', | |
HERO = 'hero-cta', | |
BANNER = 'global-banner', |
Builder.registerComponent( | ||
(props: BuilderPageTitleProps) => ( | ||
<PageTitle {...props.attributes} key={props.attributes?.key}> | ||
{props.children} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't this be props.title?
name: 'PageTitle', | ||
friendlyName: 'Page Title', | ||
noWrap: true, | ||
inputs: [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should support the variant prop here to
videoPath: props.videoPath, | ||
} | ||
|
||
const isAuthenticated = props.builderState?.state?.isAuthenticated |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This prop is not reliable enough to render authenticated content, we should use our own logic instead, like MaybeAuthenticatedContent
or LoginDialogWrapper
export interface HeroImageWrapperProps { | ||
unauthenticatedProps?: HeroImageContentProps | ||
authenticatedProps?: HeroImageContentProps | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this still necessary?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not anymore, thanks
friendlyName: 'Hero Image', | ||
canHaveChildren: false, | ||
noWrap: true, | ||
inputs: [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should also support a dynamic alt
prop here to pass to the image
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks
|
||
export function NavBarGlobalBanner({ | ||
outsideUSBannerText = 'Actions on Stand With Crypto are only available to users based in the United States.', | ||
hideBanner = false, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of adding this hideBanner
prop, can't we check if campaignText
is null and don't render the banner?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is meant to hide the banner altogether, disregarding if it has campaign or not
noWrap: true, | ||
inputs: [ | ||
{ | ||
name: 'outsideUSBannerText', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we want this prop to be controlled in Builder.io, we should check with Adam if this is necessary
- Refactored HeroBuilder and NavBarGlobalBannerBuilder to accept content prop - Updated layout and page components to pass Builder.io section content - Modified Hero and Banner components to use dynamic content - Updated section model identifiers for better clarity - Simplified component props and rendering logic
…Builder.io - Created separate HeroImage and HeroText Builder.io components - Updated page and layout components to fetch and render distinct hero content - Modified HeroImageContent props to use more generic naming - Added new render components for Hero Image and Text sections - Updated section model identifiers to distinguish between hero image and text
- Removed unused fallback prop from RenderBuilderContent component - Simplified NavBarGlobalBannerBuilder by removing fallback rendering - Cleaned up unnecessary conditional rendering logic
…dling - Added custom fetch options to getSectionContent utility function - Configured request with no-cache, keepalive, and GET method - Enhances Builder.io section content retrieval with more robust network request parameters
closes #1736
closes #1737
fixes
What changed? Why?
PlanetScale deploy request
Notes to reviewers
How has it been tested?