Skip to content
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.

Sign up for GitHub

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

Fix Page Designer ImageWithText Link component #1092

Merged
merged 6 commits into from
Apr 6, 2023
+21 −3
Merged

Fix Page Designer ImageWithText Link component #1092

Changes from 2 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
464c389
Fix PD ImageWithText Link component by using Chakra UI Link
adamraya Mar 28, 2023
b9c89c0
Use isAbsoluteURL to use react-router Link or Chakra Link component
adamraya Mar 29, 2023
77f0dc9
PR Feedback
adamraya Mar 29, 2023
bf0d7bd
Clean up
adamraya Apr 3, 2023
fa203e1
Update packages/template-retail-react-app/app/page-designer/assets/im…
adamraya Apr 3, 2023
205f320
Remove temporal page-viewer page to facilitate review
adamraya Apr 6, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 11 additions & 3 deletions packages/template-retail-react-app/app/page-designer/assets/image-with-text/index.jsx
View file
Original file line number Diff line number Diff line change
@@ -6,8 +6,16 @@
*/
import React from 'react'
import PropTypes from 'prop-types'
import {Box, Image, Text} from '@chakra-ui/react'
import {Box, Image, Link as ChakraLink, Text} from '@chakra-ui/react'
import Link from '../../../components/link'
import {isAbsoluteURL} from '../utils'

const LinkWrapper = ({ITCLink, children}) => {
const chakraLinkWrapper = (children) => <ChakraLink href={ITCLink}>{children}</ChakraLink>
const linkWrapper = (children) => <Link to={ITCLink}>{children}</Link>

return isAbsoluteURL(ITCLink) ? chakraLinkWrapper(children) : linkWrapper(children)
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you look at the Link component from the template components folder you'll see that that component has similar logic built into it that you are duplicating here maybe? 🤔

I think there is a prop called useNavLink that you might be able to take advantage of.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think that is the case.

Reading our custom link component uses a react-router Link or NavLink component (used for the Breadcrumbs menu) with the Chakra Link styles.

pwa-kit/packages/template-retail-react-app/app/components/link/index.jsx

Lines 18 to 19 in 3396472

<ChakraLink
as={useNavLink ? NavSPALink : SPALink}

There's no logic to assert if the url string is absolute, and if so, use a Chakra UI Link component instead of react-router component.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The prop useNavLink only determines if we use react-router's Link vs NavLink component.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've resolved a similar issue recently by using the as prop. Since the ...props spread is after the as={useNavLink ? ...} prop (L21 in the snippet below), you can override it by passing a custom as to the Link component.

pwa-kit/packages/template-retail-react-app/app/components/link/index.jsx

Lines 13 to 26 in 6b9d436

const Link = React.forwardRef(({href, to, useNavLink = false, ...props}, ref) => {
const _href = to || href
const {buildUrl} = useMultiSite()
const updatedHref = buildUrl(_href)
return (
<ChakraLink
as={useNavLink ? NavSPALink : SPALink}
{...(useNavLink && {exact: true})}
{...props}
to={updatedHref}
ref={ref}
/>
)
})

Meaning you could possibly adjust your current logic to something like:

import Link from '../path/to/Link'

const linkProps = ({ITCLink}) => {
    return isAbsoluteURL(ITCLink) ? { as: ChakraLink } : {}
}

() => <Link {...linkProps(/* args */ )} >...</Link>

Additionally, I added a LinkComponent?: ComponentWithAs<'a'> prop to our <Link/>, which has a default value of <ChakraLink />. This allows us to override the UI of the link (e.g. using Chakra UI's <LinkOverlay /> component) while retaining the react-router link logic from the as prop.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @breadadams for the feedback. I played with the idea and you're right, we can override our custom react-router <Link /> component logic with the ChakraLink logic by using as={ChakraLink}. The declared as prop passed to the <Link /> component will override the as prop with ...props spread.
But then we need to modify the <Link /> to conditionally use href or to props depending on if we use ChakraLink or a react-router-like Link component.

The LinkComponent idea to override styles and keep the react-router Link logic makes sense as well, but today we use the layerStyle prop pattern to override styles.

The scope of the PR is to fix the Link in the ImageWithText Page designer component. We plan to extract the Page designer code from the template in the future. In this context, I think it's better to solve the problem in the ImageWithText component file. But both ideas are good suggestions to make our custom <Link /> component more flexible in future extensibility work.


/**
* Image with text component
@@ -35,7 +43,7 @@ export const ImageWithText = ({ITCLink, ITCText, image, heading, alt}) => {
<picture>
<source srcSet={image?.src?.tablet} media="(min-width: 48em)" />
<source srcSet={image?.src?.desktop} media="(min-width: 64em)" />
<Link to={ITCLink}>
<LinkWrapper ITCLink={ITCLink}>
<Image
className={'image-with-text-image'}
data-testid={'image-with-text-image'}
@@ -45,7 +53,7 @@ export const ImageWithText = ({ITCLink, ITCText, image, heading, alt}) => {
title={alt}
filter={'brightness(40%)'}
/>
</Link>
</LinkWrapper>
</picture>
{hasCaption && (
<Text as="figcaption">
14 changes: 14 additions & 0 deletions packages/template-retail-react-app/app/page-designer/assets/utils.js
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
/*
* Copyright (c) 2023, Salesforce, Inc.
* All rights reserved.
* SPDX-License-Identifier: BSD-3-Clause
* For full license text, see the LICENSE file in the repo root or https://opensource.org/licenses/BSD-3-Clause
*/

/**
* Determines whether the specified URL is absolute
*
* @param {string} url The URL to test
* @returns {boolean} True if the specified URL is absolute, otherwise false
*/
export const isAbsoluteURL = (url) => /^([a-z][a-z\d+\-.]*:)?\/\//i.test(url)
61 changes: 61 additions & 0 deletions packages/template-retail-react-app/app/pages/page-viewer/index.jsx
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,61 @@
/*
* Copyright (c) 2023, Salesforce, Inc.
* All rights reserved.
* SPDX-License-Identifier: BSD-3-Clause
* For full license text, see the LICENSE file in the repo root or https://opensource.org/licenses/BSD-3-Clause
*/
import React from 'react'
import {Box} from '@chakra-ui/react'
import {Page, pageType} from '../../page-designer'
import {ImageTile, ImageWithText} from '../../page-designer/assets'
import {
Carousel,
MobileGrid1r1c,
MobileGrid2r1c,
MobileGrid2r2c,
MobileGrid2r3c,
MobileGrid3r1c,
MobileGrid3r2c
} from '../../page-designer/layouts'

import {HTTPError, HTTPNotFound} from 'pwa-kit-react-sdk/ssr/universal/errors'

const PAGEDESIGNER_TO_COMPONENT = {
'commerce_assets.photoTile': ImageTile,
'commerce_assets.imageAndText': ImageWithText,
'commerce_layouts.carousel': Carousel,
'commerce_layouts.mobileGrid1r1c': MobileGrid1r1c,
'commerce_layouts.mobileGrid2r1c': MobileGrid2r1c,
'commerce_layouts.mobileGrid2r2c': MobileGrid2r2c,
'commerce_layouts.mobileGrid2r3c': MobileGrid2r3c,
'commerce_layouts.mobileGrid3r1c': MobileGrid3r1c,
'commerce_layouts.mobileGrid3r2c': MobileGrid3r2c
}

const PageViewer = ({page}) => (
<Box layerStyle={'page'}>
<Page page={page} components={PAGEDESIGNER_TO_COMPONENT} />
</Box>
)

PageViewer.getProps = async ({api, params}) => {
const {pageId} = params
const page = await api.shopperExperience.getPage({
parameters: {pageId}
})

if (page.isError) {
let ErrorClass = page.type?.endsWith('page-not-found') ? HTTPNotFound : HTTPError
throw new ErrorClass(page.detail)
}

return {page}
}

PageViewer.displayName = 'PageViewer'

PageViewer.propTypes = {
page: pageType.isRequired
}

export default PageViewer
5 changes: 5 additions & 0 deletions packages/template-retail-react-app/app/routes.jsx
View file
Original file line number Diff line number Diff line change
@@ -35,6 +35,7 @@ const LoginRedirect = loadable(() => import('./pages/login-redirect'), {fallback
const ProductDetail = loadable(() => import('./pages/product-detail'), {fallback})
const ProductList = loadable(() => import('./pages/product-list'), {fallback})
const Wishlist = loadable(() => import('./pages/account/wishlist'), {fallback})
const PageViewer = loadable(() => import('./pages/page-viewer'), {fallback})
const PageNotFound = loadable(() => import('./pages/page-not-found'))

const routes = [
@@ -98,6 +99,10 @@ const routes = [
path: '/account/wishlist',
component: Wishlist
},
{
path: '/page-viewer/:pageId',
component: PageViewer
},
{
path: '*',
component: PageNotFound