-
Notifications
You must be signed in to change notification settings - Fork 55
fix(Avatar): fixing issues for the height of the element and the initials generation #38
Conversation
-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 Report
@@ 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
Continue to review full report at Codecov.
|
names = names.filter(item => item !== '') | ||
|
||
return names | ||
.map(name => (name.length ? name.charAt(0) + '.' : '')) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
name
should always have a length because of the filter above, right?
} | ||
|
||
let names = name.split(' ') | ||
names = names.filter(item => item !== '') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can avoid the let
and value reassignment by adding the filter to the chained methods:
return names
.filter( ... )
.map( ... )
.reduce( ... )
src/components/Avatar/Avatar.tsx
Outdated
const { icon = '', color = '' } = Avatar.statusToIcon[status] || {} | ||
const generateInitialsFunc = generateInitials || Avatar.generateInitials |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a good use case for defaultProps
. It will then also get typings and doc site generation.
src/components/Avatar/Avatar.tsx
Outdated
.replace(/\s*\[.*?]\s*/g, ' ') | ||
|
||
let names = name.split(' ') | ||
names = names.filter(item => item !== '') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as above, the filter can be inlined in the return chain.
src/components/Avatar/avatarRules.ts
Outdated
@@ -33,15 +33,20 @@ const getPresenceSpanLeft = (size: number, presenceIconPadding: number) => { | |||
) | |||
} | |||
|
|||
const getPresenceSpanTop = (size: number, presenceIconPadding: number) => { | |||
const getPresenceSpanTop = (size: number, presenceIconPadding: number, src: string) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's rename this method to be void of implementation (span). That will prevent future naming "breaks" due to refactors.
src/components/Avatar/avatarRules.ts
Outdated
presenceSpan: ({ props, variables }) => ({ | ||
display: 'block', | ||
position: 'relative', | ||
presenceDiv: ({ props, variables }) => ({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
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! |
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)? |
@kuzhelev the problem is that, without the introduced code
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 :) |
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. |
} | ||
|
||
const reducedName = name | ||
.replace(/\s*\(.*?\)\s*/g, ' ') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
src/components/Avatar/Avatar.tsx
Outdated
color: 'white', | ||
backgroundColor: color, | ||
...(presenceIndicatorBackground && { borderColor: presenceIndicatorBackground }), | ||
borderSize: '0.3em', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
import React from 'react' | ||
import { Avatar } from '@stardust-ui/react' | ||
|
||
const generateInitials = (name: string) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No typescript in public examples please.
.filter(item => item !== '') | ||
.map(name => `${name.charAt(0)}.`) | ||
.reduce((accumulator, currentValue) => accumulator + currentValue) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For brevity, it looks like this method can be reduced to:
const generateInitials = name => name.split(' ').map(word => `${word[0]}.`)
status="Available" | ||
/> | ||
<div style={{ background: 'white', marginBottom: '10px' }}> | ||
<div style={{ background: 'inherit', width: '10%', float: 'left', marginBottom: '10px' }}> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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" />
 
<Avatar size={size} name="John Doe" status="Available" />
 
<Avatar size={size} src="public/images/avatar/small/matt.jpg" />
</div>
)
})
export default AvatarExampleSizeShorthand
<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" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
.
@@ -1,6 +1,6 @@ | |||
import React from 'react' | |||
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" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
@@ -4,37 +4,37 @@ import { Avatar } from '@stardust-ui/react' | |||
const AvatarExampleStatusShorthand = () => ( | |||
<div style={{ background: 'white' }}> | |||
<Avatar | |||
src="/public/images/avatar/small/matt.jpg" | |||
src="public/images/avatar/small/matt.jpg" | |||
status="Available" | |||
style={{ marginRight: '15px' }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Inline styles should be replaced here. Either leave the avatars inline or use something like  
in between them to achieve spacing.
<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" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Name consideration, getInitials
?
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') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
@@ -89,55 +113,43 @@ class Avatar extends UIComponent<any, any> { | |||
} | |||
|
|||
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] || {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the review! Will try to create issues for the other things that are not part of this PR
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.