RFC: using shorthand vs rendering callback prop functions

[mnajdova]

Avatar

The motivation for this PR is to showcase some points about the different approaches we may implement for parts of the component by using shorthand and rendering callback functions. As an example, the Avatar component is used (the branch is created from #168 in order for the latest updates of the component to be reused ). The PR contains two Avatar classes (Avatar.tsx and AvatarWithoutRenderMethod.tsx). Let me briefly introduce the things that are implemented in both components.

AvatarWithoutRenderMethod

This component is improvement of the existing Avatar component that introduces shorthands for the image and label components which were previously hard-coded inside the Avatar component.

pros:

• possibility for customizing the components used inside the Avatar
• easy for understand from user perspective as the same concept is used across all high level components

cons:

• limited specter of things that can be rendered inside the Avatar component (it is ether image or label). Yes, we may use the shorthand function and introduce something like image = {() => }, but inside the function we don't have access to the properties, variables, styles inside the Avatar component. Even if we use the Provider and get access to some of those, rendering an Icon inside an image shorthand to me seems kind a awkward.

Avatar

This component, except for introducing the shorthands for the image and label, also introduces renderMethodAria prop, which is a rendering function that receives as a param the config object used inside the avatar's renderComponent method, together with the props (it is possible to include the state here as well, but there isn't state in this particular component). This method has a default implementation that has the same logic as the previous component (if the image is provided then the image is rendered, if not - then the label is rendered).

pros:

• possibility for customizing the components used inside the Avatar
• allows the user to redefine the rendering logic for parts of the Avatar completely by defining a render method where all data from the renderComponent method of the Avatar is available. If the user have seen the default implementation of the method, overriding it would be an easy thing to do.

cons:

• there are too many props for defining the same thing (we have the image and label shorthand, and we have the renderMainArea). If the image or shorthand are provided together with the renderMainArea, then it is the user's responsibility if they will use it inside the method. All these may not really be a negative thing after all, because for the users that want to have just avatar with image or name, can still have the simple API, but for the users that want some more customized Avatars, can do that as well.

Example usages:

Used code:

import React from 'react'
import { Avatar, AvatarWithoutRenderMethod, Grid, Icon } from '@stardust-ui/react'

const mapAvatarSizeToIcon = {
  1: 'micro'
  2: 'mini'
  3: 'tiny',
  4: 'small'
  5: 'normal'
  6: 'large'
  7: 'big'
  8: 'huge'
  9: 'massive'
}

const availableStatus = {
  icon: { name: 'check', variables: { color: 'white', backgroundColor: 'green' } },
  title: 'Available',
}


const renderMainArea = ({props, styles, variables}) => {
  const size = mapAvatarSizeToIcon[props.size]
  return <Icon name='home' size={size} circular inverted variables={{backgroundColor: 'blue', color: 'white'}}/>
}


const AvatarExampleNameShorthand = () => (<Grid columns='12'>
  <Avatar size='6' name="John Doe" status={availableStatus} renderMainArea={renderMainArea}/>
  <AvatarWithoutRenderMethod size='6' status={availableStatus} name="John Doe" image={() => <Icon name='home' circular inverted variables={{backgroundColor: 'blue', color: 'white'}}/>  } />
  </Grid>
  )

export default AvatarExampleNameShorthand

[codecov]

Codecov Report

Merging #199 into master will decrease coverage by 0.5%.
The diff coverage is 73.49%.

@@            Coverage Diff             @@
##           master     #199      +/-   ##
==========================================
- Coverage   67.69%   67.18%   -0.51%     
==========================================
  Files         103      106       +3     
  Lines        1461     1466       +5     
  Branches      288      281       -7     
==========================================
- Hits          989      985       -4     
- Misses        468      478      +10     
+ Partials        4        3       -1

Impacted Files	Coverage Δ
.../themes/teams/components/Avatar/avatarVariables.ts	100% <ø> (ø)	⬆️
src/index.ts	100% <100%> (ø)	⬆️
src/components/Avatar/Avatar.tsx	100% <100%> (+8.33%)	⬆️
src/themes/teams/componentVariables.ts	100% <100%> (ø)	⬆️
src/components/StatusIndicator/StatusIndicator.tsx	100% <100%> (ø)
src/themes/teams/componentStyles.ts	100% <100%> (ø)	⬆️
src/themes/teams/components/Avatar/avatarStyles.ts	27.27% <100%> (+0.8%)	⬆️
src/components/Avatar/index.ts	100% <100%> (ø)	⬆️
src/components/StatusIndicator/index.ts	100% <100%> (ø)
...onents/StatusIndicator/statusIndicatorVariables.ts	100% <100%> (ø)
... and 16 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 4697975...b032ced. Read the comment docs.

[miroslavstastny]

I like the power of renderMainArea() but I also find it difficult to use and error prone.

Implementing it is not easy - you have to correctly apply styles, accessibility, keyboard handlers... do we really want to transfer all this responsibility to users? It looks like we are building a nuclear power plant just to light up a bulb.

But I can imagine using it internally to implement 'abstract components' to be used in public components which would wrap the abstract component and implement renderXXX methods.

[musingh1]

The proposal sounds good to me.

[kuzhelov]

have discussion with @miroslavstastny, this one essentially distills to the following points.

TL;DR Takeaway

Essentially, we are still looking for the approach that will solve the following major problems

• do not introduce too much complexity/responsibility to a consumer, provide limited and only necessary set of customization tools
• if shorthand approach is considered as the solution, the problem of dealing with wrappers should be solved

Below the reasoning behind this is provided.

---

Using shorthand

• is more robust in terms of preserving component's logic (and its correctness), as there is much less customization mechanisms are provided to consumer. Also, as a consequence, there are much less things consumer should be responsible for while tweaking rendered tree - there is no need to think about event handlers or accessibility stuff (attributes, behaviors maybe more in future) should be applied, as a loud example
• current limitations:
  • it is impossible to completely alter rendered tree in cases when shorthand element will be rendered in some wrapper (this one is the case for MenuItem, where this a wrapper is used to introduce some styles) - as this wrapping element will be always rendered

<ElementType ... >
        {childrenExist(children) ? (
          children
        ) : (
          <a
            ...
            onClick={this.handleClick}
            {...accessibility.attributes.anchor}
            {...accessibility.keyHandlers.anchor}
          >
            {icon && Icon.create(...) }
            {content}
          </a>
        )}
      </ElementType>

• some names that are currently used for component parts may drive consumer in confusion, for example consider the following example. This is something that seems we will need to resolve in either case

<Avatar image={<Icon ...>}

Using 'renderXXX'

Although this one introduces all the possible customization mechanisms, it also imposes quite a lot of things that consumer should think about (styling, accessibility, potentially more will come later) - while, essentially, the only purpose of the client is to tweak rendered tree, not the behavior. There could be objections that at some cases (again, accessibility :) rendered tree might define behavior, but still it seems that we should investigate these cases more - to ensure that we are not introducing too much complexity for the client.

[mnajdova]

Absolutely agreed with your point @miroslavstastny @kuzhelov Actually maybe the best proposal would be to introduce in the components that needs something like the renderXXX methods, the following:

<ElementType ... >
        {childrenExist(children) ? (
          children
        ) : (
          <div
            ...
            {...accessibility.attributes.someComponentPart}
            {...accessibility.keyHandlers.comeComponentPart}
          >
            renderXXX({...config, props })
          </div>
        )}
      </ElementType>

With this, we would be able to handle the accessibility, behavior and all other aspect we may introduce in the future, but we would have to introduce additional element in the DOM. I can try to introduce some implementation with the proposed example if you think would be useful.

[miroslavstastny]

I don't think that would work - aria attributes must be on specific elements, you cannot move them to parent. Adding <div> in the middle is not always possible as it could break semantical structure (like ul > div > li).

[mnajdova]

So, the final decision would be for us to proceed with the usage of shorthands? Maybe we should at least try to provide the config object from the renderComponent method to the shorthand function, so that we can have the component's props, styles, variables etc... It would like something like this:

<Avatar image={({props, variables, styles}) => <Icon name='checked' color={variables.typePrimaryColor} />} />

[bcalvery]

I'm sorry I'm late to chime into this, but from the standpoint of implementing a very specific visual design, I don't see how we can legitimately restrict the ability of a person using Stardust to be unable to supply their own Image to Avatar. Absolutely (obviously?) Stardust should provide an Avatar component that is very quick and simple to consume by anyone using the shorthand, but I don't think it should restrict the possibility of someone to provide their own Image to be rendered in avatar, if that is critical to their visual design. Maybe I'm not understanding @mnajdova 's last comment though?

In the case of Microsoft Teams visual design, Avatar Image needs to have some very specific work (clipping) done that does not make sense to implement it in Stardust. The desired effect can't be accomplished by css styles alone (mostly thanks to needing to support IE/Edge), so there has to be a way to provide a user's component to avatar to be the Image.

I admit I'm still very weak in my understanding of React and Stardust at this point, but @levithomason and I talked about this last week and I think I'm articulating the idea he described to me.

[kuzhelov]

@bcalvery

I don't see how we can legitimately restrict the ability of a person using Stardust to be unable to supply their own Image to Avatar. ... I don't think it should restrict the possibility of someone to provide their own Image to be rendered in avatar, if that is critical to their visual design.

Absolutely agree - this is one of the key scenarios that needs to be supported. Please, be assured that whatever approach will be finally taken, it will support this scenario, specifically - to put some custom component into the image's slot.

Currently we have different ways to achieve this - both via shorthand, as well as via renderXXX methods. At the same time, the renderXXX option, other than supporting this scenario, provides some additional options for customizations - and this was the reason for @mnajdova to mention it in her last post. At the same time, to my mind it is a bit unclear whether we do really need these scenarios of extended customization to be supported now.

[mnajdova]

Hey guys, so, I tried using the shorthand for generating different element then the one specified and I tried adding some additional variables in the props that may be used in the shorthand method. Here is the example:

The drawback of this is that, when the image shorthand is actually used as an image, this properties are sent to the HTML dom element, because those are not handled props...

Is there any suggestion about how this should be addressed? Other then this, another thing that I have I am not much happy with, is that the props that I am receiving are for the image component, so if I want to actually get some prop, or the styles and variables for the Avatar component, I would still have to add them as some unhandled props for the Image component in order to have access to them. This will again end up with the problem I mention above. @levithomason @kuzhelov @miroslavstastny

PS: I guess I can add all these additional properties just if the shorthand for the image is function. Maybe this would kind a solve this problem?

[mnajdova]

Closing this in favor of #270