fix(Avatar): fixing issues for the height of the element and the initials generation by mnajdova · Pull Request #38 · microsoft/fluent-ui-react

mnajdova · 2018-08-01T15:43:25Z

Avatar

This PR addresses the issues created regarding the avatar components (#31 and #32)

Issue #31: Avatar status icon makes the whole component taller

Improved the style of the Avatar component so that the height would be consistent when using the presence icon.

Issue #32: Avatar initials parsing should take long / special names into account

Improved default generation of the intials by removing the content inside brackets: [], (), {} and showing just the initials of the first and last word of the name.

Other then this, there is additional property added: generateInitials for custom initial generation.

Fixing images in the avatar examples

Changed the path to the images to correspond accordingly with the images inside the Image component example

[SUGGESTION NEEDED]

There was strange need for changing the style for the avatar with size 1, containing image and presence icon. If anyone can suggest different approach or some other change, I would appreciate.

const getPresenceSpanTop = (size: number, presenceIconPadding: number, src: string) => {
  // TODO check why we need this ?!
  if (src && size === 1) {
    return getPresenceIconSize(size) * 1.5 + getPresenceIconPadding(size, presenceIconPadding) * 2
  }
  return getPresenceIconSize(size) + getPresenceIconPadding(size, presenceIconPadding)
}

-added prop generateInitials for custom logic for generating the initials -fixed style to be consistent when the presence icon is shown -fixed images in the avatar examples

codecov · 2018-08-01T16:30:00Z

Codecov Report

Merging #38 into master will increase coverage by 0.26%.
The diff coverage is 86.48%.

@@            Coverage Diff             @@
##           master      #38      +/-   ##
==========================================
+ Coverage   87.37%   87.64%   +0.26%     
==========================================
  Files          74       74              
  Lines        1125     1133       +8     
  Branches      211      213       +2     
==========================================
+ Hits          983      993      +10     
+ Misses        138      136       -2     
  Partials        4        4

Impacted Files	Coverage Δ
src/components/Icon/iconRules.ts	`75.6% <ø> (ø)`	⬆️
src/components/Icon/Icon.tsx	`100% <ø> (ø)`	⬆️
src/components/Avatar/avatarVariables.tsx	`100% <100%> (ø)`
src/components/Avatar/avatarRules.ts	`82.75% <85%> (-1.86%)`	⬇️
src/components/Avatar/Avatar.tsx	`92.1% <87.5%> (+10.85%)`	⬆️

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 c46c518...c794820. Read the comment docs.

dvdzkwsk · 2018-08-02T00:35:07Z

docs/src/examples/components/Avatar/Variations/AvatarExampleGenerateInitials.shorthand.tsx

+  names = names.filter(item => item !== '')
+
+  return names
+    .map(name => (name.length ? name.charAt(0) + '.' : ''))


name should always have a length because of the filter above, right?

levithomason · 2018-08-02T05:25:10Z

docs/src/examples/components/Avatar/Variations/AvatarExampleGenerateInitials.shorthand.tsx

+  }
+
+  let names = name.split(' ')
+  names = names.filter(item => item !== '')


You can avoid the let and value reassignment by adding the filter to the chained methods:

return names .filter( ... ) .map( ... ) .reduce( ... )

levithomason · 2018-08-02T05:26:44Z

src/components/Avatar/Avatar.tsx

-    const { src, alt, name, status } = this.props
+    const { src, alt, name, status, generateInitials } = this.props
    const { icon = '', color = '' } = Avatar.statusToIcon[status] || {}
+    const generateInitialsFunc = generateInitials || Avatar.generateInitials


This is a good use case for defaultProps. It will then also get typings and doc site generation.

levithomason · 2018-08-02T05:35:12Z

src/components/Avatar/Avatar.tsx

+      .replace(/\s*\[.*?]\s*/g, ' ')
+
+    let names = name.split(' ')
+    names = names.filter(item => item !== '')


Same as above, the filter can be inlined in the return chain.

levithomason · 2018-08-02T05:37:09Z

src/components/Avatar/avatarRules.ts

 }

-const getPresenceSpanTop = (size: number, presenceIconPadding: number) => {
+const getPresenceSpanTop = (size: number, presenceIconPadding: number, src: string) => {


Let's rename this method to be void of implementation (span). That will prevent future naming "breaks" due to refactors.

levithomason · 2018-08-02T05:38:10Z

src/components/Avatar/avatarRules.ts

-  presenceSpan: ({ props, variables }) => ({
-    display: 'block',
-    position: 'relative',
+  presenceDiv: ({ props, variables }) => ({


Avoid specifying implementation in naming as it is too tightly coupled.

-changed the names in the avatarRules, in order to avoid using specific tag names

mnajdova · 2018-08-02T10:12:01Z

Guys, I refactored the generateInitials method and put that as e default prop. Regarding the naming of the variables in the avatarRules, I changed them using words like presence indicator, wrapper and container, and now we don't have any tag name specific variables. Thanks for the review!

kuzhelov · 2018-08-02T11:33:07Z

speaking of suggestion - @mnajdova, could you, please, provide an idea about what is the goal that these changes were aimed to accomplish (problem that these are solving)?

mnajdova · 2018-08-02T14:52:19Z

@kuzhelev the problem is that, without the introduced code

const getPresenceSpanTop = (size: number, presenceIconPadding: number, src: string) => {
  // TODO check why we need this ?!
  if (src && size === 1) {
    return getPresenceIconSize(size) * 1.5 + getPresenceIconPadding(size, presenceIconPadding) * 2
  }
  return getPresenceIconSize(size) + getPresenceIconPadding(size, presenceIconPadding)
}

the presence indicator inside the Avatar has some misalignment when the size is 1:

I wasn't able to find what was causing this, that is why I added the suggestion needed help :)

levithomason · 2018-08-03T01:07:38Z

If this issue with the size === 1 is a blocker, let's add some more info to the code TODO and come back to it later.

kuzhelov · 2018-08-02T10:30:03Z

src/components/Avatar/Avatar.tsx

+      }
+
+      const reducedName = name
+        .replace(/\s*\(.*?\)\s*/g, ' ')


a bit worrying about performance implications of this - in general, we could have a linear complexity algorithm to achieve the same goal, while currently, because of regexp, we would use an algorithm of polynomial complexity (cubic at best). Taking the following things into account:

this is a logic that could be called quite often, on component's rendering

it could be the case that quite long string will be provided to the component, maybe even accidentally by setting wrong string variable as a value

<Avatar name={someWrongVariableWithLongLongText} ... />

I would rather suggest to address this issue by introducing linear algorithm for handling this aspect and not introduce possibility for bottlenecks that could surprise client of the library.

So, maybe we should create an issue and address it by means of dedicated PR

So, maybe we should create an issue and address it by means of dedicated PR

👍 to separate PR. would be good to see data before making changes.

# Conflicts: # CHANGELOG.md

-added borderSize variable and micro side for the Icon component

mnajdova · 2018-08-06T14:40:18Z

The issues for the Avatar component are now fixed, here is how the component looks:

I am using now just the icon component - the circular variation. I had to add additional size (micro) that is smaller then the rest for using it in the smallest sizes of the avatar component, as well as variable for the border size of the icon.

kuzhelov · 2018-08-06T15:24:21Z

src/components/Avatar/Avatar.tsx

+      color: 'white',
+      backgroundColor: color,
+      ...(presenceIndicatorBackground && { borderColor: presenceIndicatorBackground }),
+      borderSize: '0.3em',


a bit worrying about the fact that this is defined as constant - and then we have a line where Icon size varies depending on the size of Avatar component:

<Icon size={size < 3 ? 'micro' : size < 6 ? 'mini' : 'tiny'} ....

The problem is that Icon is designed in a way that it introduces not an absolute value for its border, but rather proportion - so, with the size of Icon changing, the border size should change as well. While currently provided approach of using relative units (em) works for font-based Icon, it won't work in case if Icon will be SVG-based - and, thus, will use absolute size units for scaling. I would rather suggest the following:

remove borderSize variable for now from Icon

let me introduce it back once SVG-based icons will be provided (there is a PR for that already)

after that we will be able to fix this aspect

What do you think?

Got it, I will revert the changes regarding the border for now, but we can create an issue in order for this to be addressed, alongside with the reimplementation of the generateInitials method.

levithomason · 2018-08-06T19:46:06Z

docs/src/examples/components/Avatar/Variations/AvatarExampleGenerateInitials.shorthand.tsx

+import React from 'react'
+import { Avatar } from '@stardust-ui/react'
+
+const generateInitials = (name: string) => {


No typescript in public examples please.

levithomason · 2018-08-06T19:53:41Z

docs/src/examples/components/Avatar/Variations/AvatarExampleGenerateInitials.shorthand.tsx

+    .filter(item => item !== '')
+    .map(name => `${name.charAt(0)}.`)
+    .reduce((accumulator, currentValue) => accumulator + currentValue)
+}


For brevity, it looks like this method can be reduced to:

const generateInitials = name => name.split(' ').map(word => `${word[0]}.`)

levithomason · 2018-08-06T19:55:36Z

docs/src/examples/components/Avatar/Variations/AvatarExampleSize.shorthand.tsx

-          status="Available"
-        />
+      <div style={{ background: 'white', marginBottom: '10px' }}>
+        <div style={{ background: 'inherit', width: '10%', float: 'left', marginBottom: '10px' }}>


It is not intuitive that these inline style background properties are required for the example to work. We don't want our examples dependent on inline styling.

Let's get this working without the inline styles. Even if we assume for now that the background color is white, it will be preferable to relying on inline styles.

As a user, I'd expect this example to work given the following the simplified example:

import _ from 'lodash' import React from 'react' import { Avatar } from '@stardust-ui/react' const AvatarExampleSizeShorthand = () => _.times(10, i => { const size = i + 1 return ( <div key={size}> <Avatar size={size} src="public/images/avatar/small/matt.jpg" status="Available" /> &emsp; <Avatar size={size} name="John Doe" status="Available" /> &emsp; <Avatar size={size} src="public/images/avatar/small/matt.jpg" /> </div> ) }) export default AvatarExampleSizeShorthand

levithomason · 2018-08-06T19:59:40Z

docs/src/examples/components/Avatar/Variations/AvatarExampleSize.shorthand.tsx

+          <Avatar key={size * 100} size={size} name="John Doe" status="Available" />
+        </div>
+        <div style={{ background: 'inherit', width: '80%', float: 'left', marginBottom: '10px' }}>
+          <Avatar key={size * 1000} size={size} src="public/images/avatar/small/matt.jpg" />


Note, key is only required by elements at the root of the array. Elements that are JSX children do not require a key.

The key can be removed from all Avatar components and instead a single key placed on the root div.

levithomason · 2018-08-06T20:02:03Z

docs/src/examples/components/Avatar/Variations/AvatarExampleSrc.shorthand.tsx

 import { Avatar } from '@stardust-ui/react'

-const AvatarExampleSrcShorthand = () => <Avatar src="/public/images/avatar/small/matt.jpg" />
+const AvatarExampleSrcShorthand = () => <Avatar src="public/images/avatar/small/matt.jpg" />


Not part of this PR, but just as something we should think about. There is no shorthand for src, so this example is not applicable. We could show shorthand by showing how the other components implement an image prop using the shorthand src value, but it might be a bit confusing.

Again, not for this PR but we can discuss this later.

levithomason · 2018-08-06T20:05:22Z

docs/src/examples/components/Avatar/Variations/AvatarExampleStatus.shorthand.tsx

-      src="/public/images/avatar/small/matt.jpg"
+      src="public/images/avatar/small/matt.jpg"
      status="Available"
      style={{ marginRight: '15px' }}


Inline styles should be replaced here. Either leave the avatars inline or use something like &emsp; in between them to achieve spacing.

levithomason · 2018-08-06T20:06:03Z

docs/src/examples/components/Avatar/Variations/index.tsx

+    <ComponentExample
+      title="Generate initials"
+      description="An Avatar can be provided with custom logic for generating the initials shown in the label."
+      examplePath="components/Avatar/Variations/AvatarExampleGenerateInitials"


Name consideration, getInitials?

levithomason · 2018-08-06T20:10:42Z

test/specs/components/Avatar/Avatar-test.tsx

+    it('generateInitials removes the text inside brackets', () => {
+      expect(Avatar.defaultProps.generateInitials('John Doe (Working position)')).toEqual('JD')
+      expect(Avatar.defaultProps.generateInitials('John Doe {Working position}')).toEqual('JD')
+      expect(Avatar.defaultProps.generateInitials('John Doe [Working position]')).toEqual('JD')


This is a neat feature, we show it off with a dedicated example. Maybe something like:

Excluded Content

Avatar initials exclude content in parens, braces, and brackets.

-removed inline styles from the examples -added excluded initials example

-added example for showing how can this background be altered with different color

kuzhelov · 2018-08-07T09:58:11Z

src/components/Avatar/Avatar.tsx

  renderComponent({ ElementType, classes, rest }) {
-    const { src, alt, name, status } = this.props
+    const { src, alt, name, status, getInitials, size } = this.props
    const { icon = '', color = '' } = Avatar.statusToIcon[status] || {}


just to not forget - there is a problem here that colors are defined based on status, and thus

there is no way to customize these colors by theming

there is no way to decouple Avatar component from the 'presenceStatus' concept - while status aspect of Avatar could be seen as something more general, not about just providing 'presence' status - it could be about providing 'role' status of the person, as a simple example.

Both these issues should be addressed by means of dedicated PRs - there is a corresponding issue created to track this work: #52

Thanks for the review! Will try to create issues for the other things that are not part of this PR

-improved logic for generating initials of the avatar

02b7247

-added prop generateInitials for custom logic for generating the initials -fixed style to be consistent when the presence icon is shown -fixed images in the avatar examples

mnajdova requested review from dvdzkwsk, kuzhelov and levithomason as code owners August 1, 2018 15:43

-added tests for the generateInitials methods im the Avatar component

829a007

mnajdova changed the title ~~[WIP] fix(Avatar): fixing issues for the height of the element and the initials generation~~ [SUGGESTION NEEDED] fix(Avatar): fixing issues for the height of the element and the initials generation Aug 1, 2018

dvdzkwsk reviewed Aug 2, 2018

View reviewed changes

levithomason reviewed Aug 2, 2018

View reviewed changes

manajdov added 2 commits August 2, 2018 12:03

-refactored generateInitials method and added as a default prop

c9b416b

-changed the names in the avatarRules, in order to avoid using specific tag names

-fixed Avatar-test.tsx

1b60a84

Merge branch 'master' into fix/avatar-issues

09de9f0

manajdov added 2 commits August 2, 2018 18:01

-added records in the changelog

2c9ad61

-removed empty line

749deef

kuzhelov reviewed Aug 3, 2018

View reviewed changes

manajdov added 2 commits August 6, 2018 10:12

Merge branch 'master' into fix/avatar-issues

20041ed

# Conflicts: # CHANGELOG.md

-replaced Avatar presence content and wrapper with circular icon

529222c

-added borderSize variable and micro side for the Icon component

mnajdova changed the title ~~[SUGGESTION NEEDED] fix(Avatar): fixing issues for the height of the element and the initials generation~~ fix(Avatar): fixing issues for the height of the element and the initials generation Aug 6, 2018

kuzhelov reviewed Aug 6, 2018

View reviewed changes

manajdov added 2 commits August 6, 2018 17:49

-removed borderSize from the icon component

52d86e1

-removed inline style in the Avatar size example

a102b66

-implementing the background of the padding as inherited

b6fe3f8

levithomason reviewed Aug 6, 2018

View reviewed changes

manajdov added 4 commits August 6, 2018 23:12

-replaced generateInitials with getInitials

501ef56

-removed inline styles from the examples -added excluded initials example

-added presence indicator background variable with default to white

36ca9dc

-added example for showing how can this background be altered with different color

Merge branch 'master' into fix/avatar-issues

63483f6

-updated changelog

c794820

kuzhelov reviewed Aug 7, 2018

View reviewed changes

kuzhelov approved these changes Aug 7, 2018

View reviewed changes

mnajdova merged commit 4d55443 into master Aug 7, 2018

levithomason deleted the fix/avatar-issues branch October 29, 2019 22:30

Conversation

mnajdova commented Aug 1, 2018

Avatar

Issue #31: Avatar status icon makes the whole component taller

Issue #32: Avatar initials parsing should take long / special names into account

Fixing images in the avatar examples

[SUGGESTION NEEDED]

Uh oh!

codecov bot commented Aug 1, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mnajdova commented Aug 2, 2018

Uh oh!

kuzhelov commented Aug 2, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mnajdova commented Aug 2, 2018

Uh oh!

levithomason commented Aug 3, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mnajdova commented Aug 6, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

levithomason Aug 6, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Excluded Content

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

codecov bot commented Aug 1, 2018 •

edited

Loading

kuzhelov commented Aug 2, 2018 •

edited

Loading

levithomason Aug 6, 2018 •

edited

Loading