# TMP Avatar Refactor & Design Prop

## Process

Refactor avatar-styles.ts into inline `design` prop overrides.

## Changes

`node_modules/@fluentui/react-bindings/dist/es/styles/types.d.ts`
- Unfortunately, we're adding types directly in node_modules to allow compiling new design prop keys

### `isCallingRosterAvatar` & `isSpeaking`

`components-calling-roster/src/calling-roster-item/calling-roster-item-components/calling-roster-item-avatar.tsx`
- Removed all `variables` since they only included these variables
- Moved avatar-styles for `isCallingRosterAvatar` and `isSpeaking` variables to `design` prop
- Added `active` prop to the user-avatar and avatar-renderer

Note, the design prop as a function is able to replace any component slot style func.
This was used along with the `:before` pseudo selector in the existing style overrides.

### `hasNoBorder`

This is pervasive across Popup components, as well as avatar.
This is a good indicator that people expect a design term for disabling borders.
Suggest we discuss adding `borderless` to our design terms, at least for Popup, despite this term explicitly describing the appearance.

Updated Avatar, did not update Popups. Share to teams affected only at time of writing 6/24.

### `isChatAvatarEmbed`

Used in embed-client in the header of a chat window with another person.
The override was for width/height of 2.8rem (28px) which is the same as `size=small` on Fluent 0.50.
However, `size=small` in Fluent <0.50 was 24px, causing the dev to override the value in order to meet redlines.

Had a designer/ux person built a blueline using real components and themes instead of creating a redline in a design tool,
they would have noticed the missing or incorrect size value right away and it would have been fixed at the source instead of the destination.

Builder example:
https://fluentsite.z22.web.core.windows.net/builder#tree_lz=N4IgLgngDgpiBcIAmBLAbiANMlBnKANgIYQByRAtnIqhtgK70pIIgBGTBSMATgLQ8A9oLBYQAEnGRYggGasAyhAptBBABQ8YRAMZgAdDAIwqAOzABKMVCFRcCUAGsYEVhxRdeA4aOw6AFh5IWqYIANqg0tQgCjAA5ma%2BOPjEZJTRsQkw5mKMzKwAzPQAnAAsAIy4SACO-gAMSZJRcorKqhpaugZGJtmW1rb28KABQSHhkdDRAGLGAB5iqCkk5FSsszALDEwsiHNEpgBMKAAchzC4AF5iTVMtiEoqapraeobGiVbYNoJ2DiBxIhQViAqD6KioegUMSjLjjeARcBTVgAQTQRDARB4izwhBW6VR6Mx2O2%2BUQAHZLpcUFCqYc2FsJFI7vIHm1np03j1PgNfkNQCgKEQ4tF-GAwHZ4AB6KVQFA6XBQ-RQfwiQS4KXlOp1AD8PAOSEE0IAvsbMJNYKwACqbJJLPFpNaIG1zJJ5XYgcmHXRQuY6apgFjYW4yVkxdkdV7dD59L4gH5-YYgHSCcx9VgAESIaGYmAABAB5YyOXAGkkgQVQQQ8TE5eCyIgEXAwbAAdxgKDiYtYzYoKHaQZAGIAsn0UKmEA2mzBTeakZbEABRVBgACSKdC2HtqVW0WXKDXG9yO1YACsUABWOYFS7lAg8P03Zmh1pPSNdd69cxxhP8kCCegwAIFBTGoMAeHoGdjQAXWwNAUWAuJQkQHQ%2Bl4EBTVg5IiDYYxdinZtsBAtBeDAGB8MbZtMNNIA

### `isHiddenParticipantsCounter`

In `chat-live-roster-renderer.tsx`, refactored to inline design prop.
This style override had a CSS selector using `[& .${Layout.slotClassNames.main}]: {...}`.
This selector is not valid since the Avatar does not contain a Layout.
It was updated when moved to the design prop to target the `[& ${Label.className}]: {...}`.

# Findings

@miro - Will need array in design prop for merging style overrides if we use them

### Prop Merging

It is difficult to merge incoming props when a component wants to take in shorthand or design props and pass them to components it is rendering.
The shorthand and design props are multi-type and can be complex to merge.
Our options are to provide mergeProps and mergeDesign functions or simplify the API to single type for each with simple pattern for merging.

There are little to no examples of shorthand literals in production (image="some-url", content="Click me").
Having variable types means difficulty in merging and more complication throughout the library.
We should move to props objects only and always.

There are in fact counter-examples of "image" props which could have been authored as taking a url but they chose to take a props object with a `url` key:

<AppTile
  image={appIcon ? { url: appIcon } : undefined}
/>

// And

<AppTile
  image={permission.appDefinition.largeImageUrl ? { url: permission.appDefinition.largeImageUrl } : undefined}
/>

Suggesting support for string shorthand, however, these were hard to find (very few of them) and could possibly be solved by ONLY allowing a string.

<Checkbox
  label="Captions"
/>

<TextField
  label="Give attendees access to support info for your organization."
/>

<Button
  content="Go to current"
  icon="call"
/>

<DefaultButton
  text="Join"  /* THIS IS NOT SHORTHAND */ 
/>

In all the code, there is a single label that uses shorthand props. This resulted in assigning `flex: 0 0 32rem` to the label:

<Checkbox
  label={{
    variables: vars("teamsAndChannels", ["teamSettingsCheckboxLabel"]),
    content:
      i18n.stringTranslate("teamsAndChannels", setting.label) +
      // This is a workaround for issue where Checkbox disabled state does not update reliably
      // for onChange events from Checkbox
      (setting.isDisabled ? " " : "")
  }}
/>

Possible conclusion, `label` and/or `text` props probably never need to be props object shorthand.
This still works with prop merging since its type is consistent.

##### More Info & Testing In v0 Docs
Merging slots:
- When composing shorthand definition, merging is required
- How does a consumer know if shorthand is a literal or object?
- If it is an object:
- className -> concat
- callbacks -> wrap, stop if
- style -> merged (deep?)
- styles -> merged (deep?)
- design -> merged (deep?)
- array?

import React from 'react'
import { Dropdown } from '@fluentui/react-northstar'

const inputItems = [
  { header: 'Bruce Wayne', image: 'public/images/avatar/small/matt.jpg', content: 'Software Engineer' },
  { header: 'Natasha Romanoff', image: 'public/images/avatar/small/jenny.jpg', content: 'UX Designer 2' },
  { header: 'Steven Strange', image: 'public/images/avatar/small/joe.jpg', content: 'Principal Software Engineering Manager' },
]

const DropdownExampleSearchMultipleImageAndContent = () => (
  <Dropdown
    multiple
    search
    items={inputItems}
    placeholder="Start typing a name"
    getA11ySelectionMessage={{
      onAdd: item => `${item.header} has been selected.`,
      onRemove: item => `${item.header} has been removed.`,
    }}
    renderSelectedItem={(Item, itemProps) => (
      <Item
        {...itemProps}
        image={{
          // How do I know this is required?!
          // What if its an obj?
          src: itemProps.image,
          children: (Image, imageProps) => (
            <Image
              // need some ugly way to give more shorthand to components, or get rid of all this nonsense.
              // from={[itemProps.image, imageProps]}
              {...imageProps}
              avatar={false}
            />
          ),
        }}
      />
    )}
    noResultsMessage="We couldn't find any matches."
  />
)

export default DropdownExampleSearchMultipleImageAndContent

### NO Design Merging [We Wish]

Note: After more refactoring, we realize this is required for now since it is less ergonomic to create multiple layers of props APIs just for triggering styles that are usually one-offs.
Nonetheless, see arguments for removing it below:

Design prop merging is an antipattern as it scatters design information throughout the tree.
Design information should be computed once at the leaf node.
Props APIs should be created to control the design of the components.

Avoiding merging drastically simplifies developer code and Fluent UI.
Fluent also can remove one layer of caching by _not_ having to merge design props (referential design prop arg checks).
Simple code is usually faster than complicated code, certainly easier to maintain.

More importantly, encapsulating design information means upgrades are easier and safer.
Conversely, spreading design information throughout the tree means brittle component internals become part of the contract and are everywhere.

We expect by forcing props APIs to be written in front of design tweaks that we will improve ability to test and document components.
This also lends itself well to future plans of having component owners or component teams own and manage component APIs, tests, docs, and designs.

### Loss of style meaning/reasoning
When migrating styles to design prop, the context and meaning of styles overrides is lost due to removing the variable name for overrides:

// ...in component
variables={{
  isCallingScreenFlexibleAvatar: size == null
}}
// ...in styles
...(isCallingScreenFlexibleAvatar && {
  height: "100%",
  overflow: "hidden",
  width: "100%"
}),

Above we know the styles are for the flexible avatar on the calling screen.
When migrated to the design prop, we loose the semantic context and only know there is a style override for a given size.
We don't know when or where this size is invoked in the app.

design={{
  ...(size == null && {
    height: "100%",
    overflow: "hidden",
    width: "100%"
  })
}}

### Selectors

Style objects should be safely typed.
Selectors need any string to work.
Selectors should then be namespaced to a special key (e.g. `$selectors: {}`) in order to preserve style key typings.

Example use of selector:

  ...(isSemoEditorAvatar && {
    "> img": {
      boxShadow: `0 0 0 0.2rem ${semoLeaseColor}`,
      padding: `0.2rem`
    },
  }),

Strawman design prop:

      design={{
        $selectors: {
          "> img": {
            boxShadow: `0 0 0 0.2rem ${semoLeaseColor}`,
            padding: `0.2rem`
          },
        },
      }}

# TODO NEXT

Test "borderless" prop added to UserAvatar. Only used in share to teams experience.
Just validate the borderless prop removes the border style.

Review TODOs added by this work. At least one requires creating a task in ADO.

# TRACING PILL IMAGE DESIGN

Dropdown
  renderSelectedItem={renderSelectedItem} /* renderSelectedItem == ListItem renderer */

renderSelectedItem(Item)
  return renderCustomSelectedItem(Item, props, cachedSuggestions, user?.id)

renderCustomSelectedItem(SelectedItem)
  return
    <UnifiedSearchDropdownSelectedItem
      SelectedItem={SelectedItem}

UnifiedSearchDropdownSelectedItem(SelectedItem)
  renderImage: (_ /*unused Image*/, imageProps)
    return getAvatarComponent(suggestion, true, imageProps, currentUserId)
  return
    <SelectedItem
      image={renderImage} // aka <ListItem image={} />
    />

getAvatarComponent(imagePropsFromDropdownSelectedItem)
  return
    <TeamAvatar
      ...imagePropsFromDropdownSelectedItem
      design... /* remove right border radius */
    />

### POSSIBLE FIXES FOR PILL DESIGN TRACING

HOW THE HELL CAN THIS BECOME SO COMPLICATED? SQUARE OFF TWO ROUND CORNERS? HACK-A-THON TO REBUILD THIS FOR UNDERSTANDING.

The above seems to want to capture the Dropdown's selected item image props and pass them to their own TeamAvatar.
Would a simple slot replacement work?

<Dropdown
  slots={{ SelectedItem: AvatarSwitch }}
/>

But, how to get all the extra props to AvatarSwitch that were built up on the way down the tree?

AvatarSwitch() {
  ...
  <TeamAvatar
    {...imageProps}
    team={suggestion.channel.team}
    size={size}
    {...(isPill && {
      image: {
        design: {
          borderTopRightRadius: "0",
          borderBottomRightRadius: "0"
        }
      }
    })}
    // variables={{ isTeamPillAvatar: isPill }}
  />
  ...
}