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

feat(Avatar): added image&label shorthands #270

Merged
merged 11 commits into from
Sep 25, 2018
Merged

Conversation

mnajdova
Copy link
Contributor

@mnajdova mnajdova commented Sep 24, 2018

Avatar

Introducing Avatar's image and label shorthands. Introduced some improvements in the styles and variables definition of the component.

TODO

  • Conformance test
  • Minimal doc site example
  • Stardust base theme
  • Teams Light theme
  • Teams Dark theme
  • Teams Contrast theme
  • Confirm RTL usage
  • W3 accessibility check
  • Stardust accessibility check
  • Update glossary props table
  • Update the CHANGELOG.md

API Proposal

image: ItemShorthand

Shorthand for image inside the Avatar component. Provided examples about how this shorthand can be used for generating Icon instead of Image.

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

const iconAsImage = (Image, props, children) => {
  return <Icon name={'user'} circular variables={{ color: 'blue' }} styles={{ fontSize: '16px' }} />
}

const AvatarExampleImageCustomizationShorthand = () => (
  <>
    <Avatar
      image={{ src: 'public/images/avatar/small/matt.jpg', alt: 'Profile picture of John Doe' }}
      status={{ color: 'green', icon: 'check', title: 'Available' }}
    />
    &emsp;
    <Avatar image={iconAsImage} status={{ color: 'green', icon: 'check', title: 'Available' }} />
  </>
)

export default AvatarExampleImageCustomizationShorthand
<div class="ui-avatar">
  <img src="public/images/avatar/small/matt.jpg" alt="Profile picture of John Doe" class="ui-image" />
  <span title="Available" class="ui-status">
    <span class="ui-icon" aria-hidden="true"></span>
  </span>
</div>
<div class="ui-avatar">
  <span class="ui-icon ab" aria-hidden="true"></span>
  <span title="Available" class="ui-status">
    <span class="ui-icon" aria-hidden="true"></span>
  </span>
</div>

image

label: ItemShorthand

Shorthand for label inside the Avatar component.

<Avatar name="John Doe" label={{ variables: { backgroundColor: 'pink' } }} />
<div class="ui-avatar">
  <div title="John Doe" class="ui-label">
    <div class="ui-layout">
      <div class="ui-layout__main">JD</div>
    </div>
  </div>
</div>

image

-fixed the variables and styles slots to be consistent with the other usages
@codecov
Copy link

codecov bot commented Sep 24, 2018

Codecov Report

Merging #270 into master will increase coverage by 0.08%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #270      +/-   ##
==========================================
+ Coverage   92.36%   92.44%   +0.08%     
==========================================
  Files          63       63              
  Lines        1152     1151       -1     
  Branches      152      172      +20     
==========================================
  Hits         1064     1064              
  Misses         84       84              
+ Partials        4        3       -1
Impacted Files Coverage Δ
src/components/Avatar/Avatar.tsx 100% <100%> (+3.33%) ⬆️

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 d548a33...3a181c0. Read the comment docs.

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

const iconAsImage = (Image, props, children) => {
return <Icon name={'user'} circular variables={{ color: 'blue' }} styles={{ fontSize: '16px' }} />
Copy link
Contributor

Choose a reason for hiding this comment

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

seems that the expression for name prop could be safely simplified to use just quotes

},
})}
{!image &&
Label.create(label || {}, {
Copy link
Contributor

Choose a reason for hiding this comment

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

currently it is possible to use component in the following way:

<Avatar name='John Doe' label={{ content: 'A' }} />

In that case the name's value will be completely ignored. Could we consider name to be a shorthand slot (with Label being a default component)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The name and label are two different things. The name is used as a title for the Label and Image components inside the Avatar, but the content of the Label is actually the result of getInitials method (prop), that uses the names for generating the initials. The use can however override the content of the Label using the shorthand, and the name can still reflects the actual user's name.

Copy link
Contributor

@kuzhelov kuzhelov Sep 24, 2018

Choose a reason for hiding this comment

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

frankly, a bit unsure whether we do need both label and name at the same time - it seems that for almost all cases it would be sufficient to have just one prop. Let's please, discuss this tomorrow

statusBorderColor: 'white',
statusBorderWidth: 2,
label: { padding: '0px' },
status: {
Copy link
Contributor

Choose a reason for hiding this comment

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

I would rather suggest to refrain from utilizing this pattern of nested (child) variables for now, as we are not completely sure if it is a way to go for us (couple of pending prototypes are under review currently). It seems that we might consider to hardcode these styles for now (in avatarStyles file) - although I do understand that we will lose a bit in terms of customization capabilities for the examples, at the same time we

  • won't restrict the functionality for Teams as a consumer
  • would be at much safer spot in terms of using only the approaches that we have agreed upon

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, will try to replace then all the variables that are referring some components slots with the slot's inside the styles.

Copy link
Contributor Author

@mnajdova mnajdova Sep 24, 2018

Choose a reason for hiding this comment

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

I reverted the changes I introduced for the status slot, and removed the label's variables. I wasn't able to get rid of the variables for the status, as these variables are used in multiple places for different calculation inside the statusStyles.

@kuzhelov kuzhelov added needs author feedback Author's opinion is asked and removed 🚀 ready for review labels Sep 24, 2018
@mnajdova mnajdova added 🚀 ready for review and removed needs author feedback Author's opinion is asked labels Sep 24, 2018
@mnajdova mnajdova merged commit b71d78d into master Sep 25, 2018
@levithomason levithomason deleted the feat/avatar-shorthands branch September 25, 2018 19:29
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants