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

fix(avatar): font icons sizing issue #846

Closed
wants to merge 2 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ const AvatarExampleImageCustomizationShorthand = () => (
image={
// This example does not react to the avatar size variable
// and otherwise produces bad results when border is applied compared to "normal" image
<Icon name="user" circular variables={{ color: 'blue' }} styles={{ padding: '8px' }} />
<Icon name="user" circular variables={{ color: 'blue' }} />
}
status={{ color: 'green', icon: 'check', title: 'Available' }}
/>
Expand Down
9 changes: 9 additions & 0 deletions packages/react/src/components/Avatar/Avatar.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,10 @@ import {
SizeValue,
} from '../../lib'

export type AvatarSlotClassNames = {
image: string
}

export interface AvatarProps extends UIComponentProps {
/** Shorthand for the image. */
image?: ShorthandValue
Expand Down Expand Up @@ -43,6 +47,10 @@ class Avatar extends UIComponent<ReactProps<AvatarProps>, any> {

static displayName = 'Avatar'

static slotClassNames = {
image: `${Avatar.className}__image`,
}

static propTypes = {
...commonPropTypes.createCommon({
children: false,
Expand Down Expand Up @@ -92,6 +100,7 @@ class Avatar extends UIComponent<ReactProps<AvatarProps>, any> {
avatar: true,
title: name,
styles: styles.image,
className: Avatar.slotClassNames.image,
},
})}
{!image &&
Expand Down
8 changes: 6 additions & 2 deletions packages/react/src/lib/callable.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,10 @@
import { Func } from '../types'

// https://jsperf.com/startdust-callable
Copy link
Contributor Author

Choose a reason for hiding this comment

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

we may safely remove this reference as

  • it is pointing to stale document
  • we have all the necessary tooling now to measure performance on the Stardust's side

Copy link
Member

Choose a reason for hiding this comment

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

Let's remove it 👍

const callable = (possibleFunction: any) => (...args: any[]) => {
return typeof possibleFunction === 'function' ? possibleFunction(...args) : possibleFunction
const callable = <T = {}>(possibleFunction: T | Func<T>) => (...args: any[]) => {
return typeof possibleFunction === 'function'
? (possibleFunction as Func<T>)(...args)
: possibleFunction
}

export default callable
4 changes: 2 additions & 2 deletions packages/react/src/lib/createAnimationStyles.tsx
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import { ThemePrepared, AnimationProp } from '../themes/types'
import { AnimationProp, ICSSInJSStyle, ThemePrepared } from '../themes/types'
import callable from './callable'

const createAnimationStyles = (animation: AnimationProp, theme: ThemePrepared) => {
const createAnimationStyles = (animation: AnimationProp, theme: ThemePrepared): ICSSInJSStyle => {
let animationCSSProp = {}
const { animations = {} } = theme

Expand Down
5 changes: 4 additions & 1 deletion packages/react/src/lib/getClasses.ts
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,10 @@ const getClasses = (
const componentParts: string[] = Object.keys(componentStyles)

return componentParts.reduce((classes, partName) => {
classes[partName] = renderer.renderRule(callable(componentStyles[partName]), styleParam)
// The reason for explicit cast is that Fela and Stardust use different interfaces for CSS styles
// - Fela: interface IStyle extends CSS.Properties<string | number>
// - Stardust: interface ICSSInJSStyle extends React.CSSProperties
classes[partName] = renderer.renderRule(callable<any>(componentStyles[partName]), styleParam)

return classes
}, {})
Expand Down
4 changes: 2 additions & 2 deletions packages/react/src/lib/renderComponent.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -211,10 +211,10 @@ const renderComponent = <P extends {}>(config: RenderConfig<P>): React.ReactElem
colors,
}

mergedStyles.root = {
mergedStyles.root = callable({
...callable(mergedStyles.root)(styleParam),
...animationCSSProp,
}
})

const resolvedStyles: ComponentSlotStylesPrepared = Object.keys(mergedStyles).reduce(
(acc, next) => ({ ...acc, [next]: callable(mergedStyles[next])(styleParam) }),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ export { default as Chat } from './components/Chat/chatVariables'
export { default as ChatMessage } from './components/Chat/chatMessageVariables'
export { default as Divider } from './components/Divider/dividerVariables'
export { default as Header } from './components/Header/headerVariables'
export { default as Icon } from './components/Icon/iconVariables'
export { default as Input } from './components/Input/inputVariables'
export { default as Menu } from './components/Menu/menuVariables'
export { default as Text } from './components/Text/textVariables'
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
import avatarVariables from '../Avatar/avatarVariables'
import { callable } from '../../../../lib'

export default siteVars => ({
avatarBorderWidth: callable(avatarVariables)(siteVars).avatarBorderWidth,
})
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
import { pxToRem } from '../../../../lib'
import { ComponentSlotStylesInput, ICSSInJSStyle } from '../../../types'
import { AvatarProps } from '../../../../components/Avatar/Avatar'
import { AvatarProps } from '../../../..'
Copy link
Member

Choose a reason for hiding this comment

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

Can we still use the direct import?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sure, thanks for catching this thing!


const sizeToPxValue = {
export const sizeToPxValue = {
smallest: 24,
smaller: 24,
small: 24,
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
import * as _ from 'lodash'

import { pxToRem, SizeValue } from '../../../../lib'
import { IconVariables } from './iconVariables'

import { sizeToPxValue as avatarSizeToPxValue } from '../Avatar/avatarStyles'

const sizeToIconPaddingInPx: Record<SizeValue, number> = {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

these values need to be refined

smallest: 4,
smaller: 4,
small: 4,
medium: 6,
large: 8,
larger: 8,
largest: 10,
}

export const getAvatarFontIconStyles = (size: SizeValue, v: IconVariables) => {
const slotSizeInPx = avatarSizeToPxValue[size]
const iconBorderWidthInPx = v.avatarBorderWidth || 0
const iconPaddingSizeInPx = sizeToIconPaddingInPx[size]

const iconSizeInRems = pxToRem(slotSizeInPx - 2 * iconPaddingSizeInPx - 2 * iconBorderWidthInPx)

return {
fontSize: iconSizeInRems,
padding: pxToRem(iconPaddingSizeInPx),

'::before': {
width: iconSizeInRems,
height: iconSizeInRems,
},
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -4,9 +4,10 @@ import fontAwesomeIcons from './fontAwesomeIconStyles'
import { callable, pxToRem, SizeValue } from '../../../../lib'
import { ComponentSlotStylesInput, ICSSInJSStyle, FontIconSpec } from '../../../types'
import { ResultOf } from '../../../../types'
import { IconXSpacing, IconProps } from '../../../../components/Icon/Icon'
import { Avatar, IconXSpacing, IconProps } from '../../../../'
import { getStyle as getSvgStyle } from './svg'
import { IconVariables, IconSizeModifier } from './iconVariables'
import { getAvatarFontIconStyles } from './avatarIconStyles'

const sizes: Record<SizeValue, number> = {
smallest: 7,
Expand All @@ -19,7 +20,7 @@ const sizes: Record<SizeValue, number> = {
}

const getDefaultFontIcon = (iconName: string) => {
return callable(fontAwesomeIcons(iconName).icon)()
return callable(fontAwesomeIcons(iconName).icon as FontIconSpec)()
}

const getFontStyles = (
Expand Down Expand Up @@ -124,6 +125,10 @@ const iconStyles: ComponentSlotStylesInput<IconProps, IconVariables> = {
...(!rtl && {
transform: `rotate(${rotate}deg)`,
}),

[`&.${Avatar.slotClassNames.image}`]: {
...(isFontBased && getAvatarFontIconStyles(size, v)),
},
Copy link
Member

Choose a reason for hiding this comment

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

Actually, this is a single thing that stops me from the approve button.

I understand why Avatar can know about Icon, because it's a slot of Avatar. But, I want to avoid the reserve coupling because Icon is a primitive component  🤔

}
},

Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
import { ColorValues } from '../../../types'
import { mapColorsToScheme, pxToRem } from '../../../../lib'
import { callable, mapColorsToScheme, pxToRem } from '../../../../lib'

import avatarVariables from '../Avatar/avatarVariables'

export type IconSizeModifier = 'x' | 'xx'

Expand All @@ -19,6 +21,8 @@ export interface IconVariables {
marginRight: string
outline?: boolean
sizeModifier?: IconSizeModifier

avatarBorderWidth?: number
}

const colorVariant = 500
Expand All @@ -36,4 +40,6 @@ export default (siteVars): IconVariables => ({
horizontalSpace: pxToRem(10),
marginRight: pxToRem(8),
outline: undefined,

avatarBorderWidth: callable(avatarVariables)(siteVars).avatarBorderWidth,
})
1 change: 1 addition & 0 deletions packages/react/src/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ export type ResultOf<T> = T extends (...arg: any[]) => infer TResult ? TResult :
export type OneOrArray<T> = T | T[]
export type ObjectOf<T> = { [key: string]: T }

export type Func<TResult> = (...args: any[]) => TResult
export type ObjectOrFunc<TResult, TArg = {}> = ((arg: TArg) => TResult) | TResult

// ========================================================
Expand Down