fix(Avatar): alignment of status icon on size changes #828

kuzhelov · 2019-02-01T17:32:11Z

TODO

introduce breaking change entry to CHANGELOG

Breaking Change

This PR unifies the set of values that could be provided to size prop of Avatar and Status components: instead of numbers that were originally used, now there is a finite set of string values that could be provided as size:

type SizeValue = 'smallest' | 'smaller' | 'small' | 'medium' | 'large' | 'larger' | 'largest'

Necessary changes on the client side

This PR requires the following changes to be made on the client side at places where size values were provided to either Avatar component, or to the size prop of its status slot. No changes should be introduced in case if no any value was provided to size prop.

`sizes` of Avatar and `status` slot originally were not in sync

Size of status icon was always remained the same and haven't responded to Avatar's size changes - now size of status slot changes alongside with changes to Avatar's size:

<Avatar size='larger' status={{ icon: 'check', color: 'green' }} image={...} />

Was

Now

In order to preserve original behavior size of status slot must be set explicitly:

<Avatar size='larger' status={{ size='medium', ... }} image={...} />

`Avatar`, value was provided to `size` prop

// Before
<Avatar size={32} />

// After
<Avatar size='medium' />

`Avatar`, `size` value is provided to `status` slot

// Before
<Avatar status={{ size: 10, ... }}/>

// After
<Avatar status={{ size: 'medium', ... }}/>

Removed types

AvatarPropsWithDefaults and StatusPropsWithDefaults types were removed from public API. Client should replace them with AvatarProps and StatusProps at places of usage, accordingly.

Description

This PR unifies size values for Avatar component - now those values are taken from common Stardust dictionary (smallest -> ... -> largest).

Related styles of Teams theme were adjusted accordingly.

Fix: unresponsiveness of `status` slot to size changes

Was

Now

…n-tests

…ttps://github.com/stardust-ui/react into feat/add-ts-support-to-screener-interaction-tests

Co-Authored-By: kuzhelov <deckster@mail.ru>

…n-tests

codecov · 2019-02-01T17:39:56Z

Codecov Report

Merging #828 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master     #828   +/-   ##
=======================================
  Coverage   93.54%   93.54%           
=======================================
  Files          21       21           
  Lines         728      728           
  Branches       69       69           
=======================================
  Hits          681      681           
  Misses         47       47

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 faf1e2a...d53a6ec. Read the comment docs.

layershifter · 2019-02-04T15:25:25Z

packages/react/src/themes/teams/components/Status/statusStyles.ts

+}
+
+export const getSizeStyles = (sizeInPx: number, variables: StatusVariables) => {
+  const sizeInRem = pxToRem(sizeInPx + 2 * ((variables.borderColor && variables.borderWidth) || 0))


Suggested change

const sizeInRem = pxToRem(sizeInPx + 2 * ((variables.borderColor && variables.borderWidth) || 0))

const borderWidth = (variables.borderColor && variables.borderWidth) || 0

const sizeInRem = pxToRem(sizeInPx + 2 * borderWidth)

One line will not save us 😸 Can we make it more readable?

layershifter · 2019-02-04T15:27:55Z

packages/react/src/themes/teams/components/Icon/iconStyles.ts

 import { getStyle as getSvgStyle } from './svg'
 import { IconVariables, IconSizeModifier } from './iconVariables'

-const sizes: { [key in IconSize]: number } = {
+const sizes: { [key in SizeValue]: number } = {


Suggested change

const sizes: { [key in SizeValue]: number } = {

const sizes: Record<SizeValue, number> = {

As I remember this should work

layershifter · 2019-02-04T15:28:26Z

packages/react/src/themes/teams/components/Status/statusStyles.ts

@@ -35,16 +35,33 @@ const getTextColor = (state: string, variables: StatusVariables) => {
  }
 }

-const statusStyles: ComponentSlotStylesInput<StatusPropsWithDefaults, StatusVariables> = {
+export const sizeToPxValue = {


Can we introduce a type there, i.e.:

Suggested change

export const sizeToPxValue = {

export const sizeToPxValue: Record<SizeValue, number> = {

layershifter · 2019-02-04T15:31:03Z

packages/react/src/index.ts

@@ -120,7 +120,7 @@ export {
 export { default as Ref, RefProps } from './components/Ref/Ref'
 export { default as Segment, SegmentProps } from './components/Segment/Segment'

-export { default as Status, StatusPropsWithDefaults, StatusProps } from './components/Status/Status'
+export { default as Status, StatusProps } from './components/Status/Status'


StatusPropsWithDefaults was part of public API, so this is a breaking change, too

mnajdova · 2019-02-04T16:59:56Z

packages/react/src/components/Avatar/Avatar.tsx

@@ -23,10 +24,10 @@ export interface AvatarProps extends UIComponentProps {
  name?: string

  /** Size multiplier. */
-  size?: number
+  size?: SizeValue


Can you check if there are some usages in the prototypes (I am pretty sure there is employee card prototype that has bigger Avatar)

sure, thanks for heads up!

mnajdova · 2019-02-04T17:00:10Z

packages/react/src/components/Avatar/Avatar.tsx


  /** Shorthand for the status of the user. */
-  status?: ShorthandValue
+  status?: ShorthandValue<StatusProps>


mnajdova

LGTM please just check the prototypes if there are some miss-usages of the size props after these changes.

… into fix/avatar-sizes

kuzhelov-ms and others added 13 commits January 31, 2019 20:22

add TS support

f763c05

fix port value

a538f4f

adjust host URL values

ea146c5

Merge branch 'master' into feat/add-ts-support-to-screener-interactio…

5cb7fa8

…n-tests

simplify steps extraction logic

b52c2f6

nit: improve readability

cf5785e

Merge branch 'feat/add-ts-support-to-screener-interaction-tests' of h…

e90f525

…ttps://github.com/stardust-ui/react into feat/add-ts-support-to-screener-interaction-tests

Update screener.config.js

cb5f3f0

Co-Authored-By: kuzhelov <deckster@mail.ru>

fix merge problem

013019b

Merge branch 'master' into feat/add-ts-support-to-screener-interactio…

2e3ad45

…n-tests

upd

8f92180

fix styles for different avatar sizes

15979c5

Merge branch 'master' into fix/avatar-sizes

6a6e1c8

kuzhelov requested review from dvdzkwsk, layershifter, levithomason, miroslavstastny and mnajdova as code owners February 1, 2019 17:32

kuzhelov added 🚀 ready for review 🧰 fix Introduces fix for broken behavior. labels Feb 1, 2019

kuzhelov-ms and others added 3 commits February 1, 2019 20:17

unify size values for Loader and Icon

6fa5d8d

changelog

81cf7d3

Merge branch 'master' into fix/avatar-sizes

5327a12

layershifter reviewed Feb 4, 2019

View reviewed changes

apply review comments

a621726

mnajdova reviewed Feb 4, 2019

View reviewed changes

mnajdova approved these changes Feb 4, 2019

View reviewed changes

kuzhelov added the needs author feedback Author's opinion is asked label Feb 4, 2019

kuzhelov-ms and others added 2 commits February 5, 2019 02:03

fix employee card prototype

6673d55

Merge branch 'master' into fix/avatar-sizes

6726979

kuzhelov added ready for merge and removed needs author feedback Author's opinion is asked labels Feb 5, 2019

kuzhelov-ms and others added 3 commits February 5, 2019 10:26

upd changelog

2085e7d

Merge branch 'fix/avatar-sizes' of https://github.com/stardust-ui/react…

cf23a0d

… into fix/avatar-sizes

Merge branch 'master' into fix/avatar-sizes

d53a6ec

kuzhelov merged commit aefc1fa into master Feb 5, 2019

kuzhelov deleted the fix/avatar-sizes branch February 5, 2019 11:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(Avatar): alignment of status icon on size changes #828

fix(Avatar): alignment of status icon on size changes #828

kuzhelov commented Feb 1, 2019 •

edited

codecov bot commented Feb 1, 2019 •

edited

layershifter Feb 4, 2019

layershifter Feb 4, 2019

layershifter Feb 4, 2019

layershifter Feb 4, 2019

mnajdova Feb 4, 2019

kuzhelov Feb 5, 2019

mnajdova Feb 4, 2019

mnajdova left a comment

	const sizeInRem = pxToRem(sizeInPx + 2 * ((variables.borderColor && variables.borderWidth) \|\| 0))
	const borderWidth = (variables.borderColor && variables.borderWidth) \|\| 0
	const sizeInRem = pxToRem(sizeInPx + 2 * borderWidth)

	const sizes: { [key in SizeValue]: number } = {
	const sizes: Record<SizeValue, number> = {

	export const sizeToPxValue = {
	export const sizeToPxValue: Record<SizeValue, number> = {

fix(Avatar): alignment of status icon on size changes #828

fix(Avatar): alignment of status icon on size changes #828

Conversation

kuzhelov commented Feb 1, 2019 • edited

TODO

Breaking Change

Necessary changes on the client side

sizes of Avatar and status slot originally were not in sync

Was

Now

Avatar, value was provided to size prop

Avatar, size value is provided to status slot

Removed types

Description

Fix: unresponsiveness of status slot to size changes

Was

Now

codecov bot commented Feb 1, 2019 • edited

Codecov Report

layershifter Feb 4, 2019

Choose a reason for hiding this comment

layershifter Feb 4, 2019

Choose a reason for hiding this comment

layershifter Feb 4, 2019

Choose a reason for hiding this comment

layershifter Feb 4, 2019

Choose a reason for hiding this comment

mnajdova Feb 4, 2019

Choose a reason for hiding this comment

kuzhelov Feb 5, 2019

Choose a reason for hiding this comment

mnajdova Feb 4, 2019

Choose a reason for hiding this comment

mnajdova left a comment

Choose a reason for hiding this comment

kuzhelov commented Feb 1, 2019 •

edited

`sizes` of Avatar and `status` slot originally were not in sync

`Avatar`, value was provided to `size` prop

`Avatar`, `size` value is provided to `status` slot

Fix: unresponsiveness of `status` slot to size changes

codecov bot commented Feb 1, 2019 •

edited