Skip to content

[react] Refactor components and HOCs#367

Open
art-alexeyenko wants to merge 11 commits intodevfrom
feature/jss-8654-react-best
Open

[react] Refactor components and HOCs#367
art-alexeyenko wants to merge 11 commits intodevfrom
feature/jss-8654-react-best

Conversation

@art-alexeyenko
Copy link
Contributor

  • This PR follows the Contribution Guide
  • Changelog updated - TBD
  • Upgrade guide entry added

Description / Motivation

Refactors react package with simplicity and best React practices in mind.

  • Convert all built in components to functions
  • Make them use React hooks where appropriate
  • Use JSX logic instead of React.createElement
  • Major Placeholder, AppPlaceholder rewrite:
    • align logic between the two
    • convert Placeholder to function component
    • rework the logic of passing props to components, only pass down what's neccessary
    • introduce passThroughProps prop to add flexibility in passing props to child components
    • fix renderEach (Fix behaviour of renderEach in page editing mode #364) Thanks @liamhubers
    • general refactoring
  • Remove withPlaceholder and replace it with withClientPlaceholder - a clientside/SSR analogue to withAppPlaceholder
  • Add a useComponentMap hook to be used instead of withComponentMap when needed.
  • FEAAS/BYOC logic remains unchanged for compatibility with components logic

Testing Details

  • Unit Test Added
  • Manual Test/Other (Please elaborate)

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

@@ -21,124 +18,67 @@ import { rsc } from '#rsc-env';
* @public
*/
export const AppPlaceholder = (props: AppPlaceholderProps) => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

AppPlaceholder was designed to work universally in App router and Pages router, with only new requirement - have componentMap and page provided as required props.
Can we make Placeholder just as wrapper over AppPlaceholder? Smth like:

const Placeholder = (props)=>{
  const { componentMap, page } = useSitecore();
  const appProps = {
     ...props,
     componentMap
     page
  }
  return <AppPlaceholder {...appProps} />
}

If not, what behavior we expect Placeholder to have that is not present in AppPlaceholder?

};
};

export const drawPlaceholderComponents = (
Copy link
Collaborator

Choose a reason for hiding this comment

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

This function accepts some props and returns back JSX, why won't you make it as React component?
It also accepts drawPlaceholderChildComponent function as parameter, which is also accepts some props and returns JSX, so it is could also be a component passed as parameter. But I think it is possible to make it as regular childern prop

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants