Skip to content
This repository has been archived by the owner on Mar 4, 2020. It is now read-only.

BREAKING: add render* props for each shorthand prop #328

Merged
merged 8 commits into from
Oct 7, 2018

Conversation

levithomason
Copy link
Member

@levithomason levithomason commented Oct 5, 2018

Before

Shorthand used to accept a render function:

<Button 
  icon={(Icon, props) => <p><Icon {...props} /></p>}
/>

After

Dedicated render* props have been added for each shorthand prop:

<Button 
  icon={string|props}
  renderIcon={(Icon, props) => <p><Icon {...props} /></p>}
/>

This allows shorthand data to be passed and processed while simultaneously allowing a custom render tree of that preprocessed data.


Currently, it is not possible to define both shorthand data and a render function for components that take an array of shorthand. The array elements are used as values for the shorthand. Passing a render function for a given shorthand array element means that array element now cannot be a value, such as a props object.

What we need is a way to iterate over the shorthand items with a render function. This way, the factory can still normalize the shorthand values to Component and props, the parent component can compute state, styling, and accessibility, and the user gets to construct the tree using all this information:

<Menu
  items={[
    'Home',
    { icon: 'user', content: 'Profile' },
  ]}

  renderItem={(Component, props) => (
    // Component : computed component for this menu item
    // props     : computed menu item props including state, styling, and accessibility
    <Component {...props} />
  )}
/>

This format gives consumers the ability to simultaneously:

  1. Define shorthand data (strings, numbers, props objects)
  2. Control the render tree of the shorthand components
  3. Accept and use state, styling, and accessibility computed by the Menu
  4. Asynchronously render child shorthand components

This pattern needs to apply to all our components and I have done so in this PR.

@codecov
Copy link

codecov bot commented Oct 5, 2018

Codecov Report

Merging #328 into master will increase coverage by 0.23%.
The diff coverage is 97.26%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #328      +/-   ##
==========================================
+ Coverage   89.32%   89.55%   +0.23%     
==========================================
  Files          62       62              
  Lines        1180     1216      +36     
  Branches      176      178       +2     
==========================================
+ Hits         1054     1089      +35     
- Misses        124      125       +1     
  Partials        2        2
Impacted Files Coverage Δ
src/components/Portal/Portal.tsx 100% <ø> (ø) ⬆️
src/components/Provider/ProviderConsumer.tsx 100% <ø> (ø) ⬆️
src/components/Grid/Grid.tsx 100% <ø> (ø) ⬆️
src/components/Popup/Popup.tsx 41.37% <ø> (ø) ⬆️
src/components/Menu/MenuItem.tsx 80% <100%> (ø) ⬆️
src/components/Avatar/Avatar.tsx 100% <100%> (ø) ⬆️
src/components/Button/Button.tsx 95.74% <100%> (ø) ⬆️
src/components/Input/Input.tsx 84.61% <100%> (ø) ⬆️
src/components/List/List.tsx 100% <100%> (ø) ⬆️
src/components/Status/Status.tsx 100% <100%> (ø) ⬆️
... and 10 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 03832b4...5464c28. Read the comment docs.

@levithomason levithomason changed the title feat: add render* props for each shorthand prop BREAKING: add render* props for each shorthand prop Oct 5, 2018
Component: React.ReactType,
props: IProps,
children: ReactChildren,
) => React.ReactElement<any>
Copy link
Member

Choose a reason for hiding this comment

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

SUIR now has following typings:

export type SemanticShorthandItemFunc<TProps> = (
  component: React.ComponentType<TProps>,
  props: TProps,
  children?: React.ReactChildren,
) => React.ReactElement<any> | null

This allows to access passed props.

peek 2018-09-30 13-26

Copy link
Member Author

Choose a reason for hiding this comment

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

Nice. We were just discussing improving typings for shorthand in Stardust as well.

Note on SUIR's typings here:

  • React.ComponentType only includes Component and SFC but not strings ('div', etc). Use React.ReactType to include all valid component types.
  • React.ReactChildren is actually the API for the React.Children utility. Use React.ReactNodeArray | React.ReactNode for return values from render functions.

@kuzhelov
Copy link
Contributor

kuzhelov commented Oct 10, 2018

Problems

There are several problems with the approach provided, though - let me mention them.

1. Points of general confusion for client

  • if renderItem function is provided, items value doesn't represent shorthands' array, strictly speaking, as something as the following one becomes to be absolutely legal.
// lets just provide a list of urls :)
<Menu items={[ 'http://..', .. ]} renderItem={.. process URL and render tree ..}

It means that in some cases items is a shorthand array, but with renderItem function being specified - it could be just some arbitrary data array. However, there is a downside that this array will be merged to the props of the rendered item.

  • how shorthand's object data is passed to renderItem? (it turns out to be inside props)
<Menu items={[ 
  { key: ..., ... }
  ...
]} 
renderItem={(Component, props) => ... where items data comes to? .. }

2. "Do It Yourself" pattern provided by renderItem to compose component.

Lets consider the API we now have for renderItem:

<Menu items={items} renderItem={(Component, props) => {
   <Async 
     getData={() => ...}
     onResolve={data => 
       <Component {...props}>    <===== client's responsibility now
          {props.children}  <==== client's responsibility now
       </Component>}
   />
}/>

As we might see, there are quite a lot of things that client should worry about now - and, as we might see, this approach is quite error-prone for the client.

3. Shorthand is evaluated at different stage for async case.

Lets consider synchronous rendering case:

const items = [
  { key: '...', content: '...' },
  .. 
]


<Menu items={items} ... />

One thing that we might observe here is that shorthand item is rendered after its descriptor object (i.e. element of items array) is fully initialized.

Now lets move to async case with renderItem approach, note that Component evaluation now happens before data is fetched. To comply with the semantics of shorthand we've had for sync case, we need this evaluation to happen after necessary data is fetched - and this might be absolutely necessary if shorthand is using some data that is fetched asynchronously for the sake of being properly rendered.

const items = [ .. partially initialized data ..]
<Menu items={items} renderItem={(Component, props) => {
   <Async 
     getData={() => ...}
     onResolve={data => <Component {props}></Component>}
   />
}/>

Proposed Solution

Idea

Let me, please, propose the alternative solution that aims to

  • solve the problems highlighted
  • meet all the merit points that are mentioned above.
  • Define shorthand data (strings, numbers, props objects)
  • Control the render tree of the shorthand components
  • Accept and use state, styling, and accessibility computed by the Menu
  • Asynchronously render child shorthand components

Core idea is that while working with async cases, we, as a client, are interested just to solve the following tasks:

  • delay the moment of when shorthand item is rendered
  • place rendered shorthand item at the proper place in the component's tree

And that's it. So, what if we would provide this recipe of shorthand rendering to the client?

Example

/**
/* - 'items' still return components array to render
/* - no need to use 'renderItem'
**/
<Menu items={(renderItemAt) => [   // <--------------------- and let client decide where to use it
   <Async
    getData={() => ... fetch data asynchronously .. }
    render={(data) => {
       const shorthand = // <----------- preprocess fetched data, build shorthand descriptor
       return (
       <div>
           renderItemAt(0, shorthand)
       </div>
    } 
  />,
  ...
]}

Notes

Note that this approach could be generalized for all shorthands - and, thus, for all slots we have.

As a final note, if client is interested in synchronous rendering, the very same approach that was used before could be considered:

<Menu items={[
  { key: ..., }
  ...
]} .. />

@kuzhelov kuzhelov mentioned this pull request Oct 19, 2018
11 tasks
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants