Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add <Avatar> to display an image along with the text #208

Merged
merged 15 commits into from
May 3, 2019
Merged

Conversation

hsunpei
Copy link
Contributor

@hsunpei hsunpei commented Apr 26, 2019

Purpose

In our project in development, there is a need to add users' avatars alongside the text of <ListRow>, <SelectRow>, and <SelectOption>.

With this in mind, we add the following three types (square, rounded, and circle) of <Avatar>s to display an image along with the text, and change related components to accommodate the avatars.

Screen Shot 2019-04-26 at 6 24 45 PM

Screenshots

<Avatar>

Screen Shot 2019-04-26 at 6 27 39 PM

<ListRow>

Screen Shot 2019-04-26 at 6 29 26 PM

<Checkbox>

Screen Shot 2019-04-26 at 6 30 27 PM

<SelectRow>

Kapture 2019-04-26 at 18 32 54

Changes

  • Add <Avatar>
  • Change rowComp() to allow the appearance of <Avatar> alongside the text
  • Change <SelectRow> and <Checkbox> to display <Avatar>

Risks

The change of rowComp() allows any components wrapped with it to integrate <Avatar> on its left. However, since <Avatar> is an experimental feature in a project, components except<ListRow> and <SelectRow> have not been tested and documented.

@hsunpei hsunpei requested review from zhusee2 and tz5514 April 26, 2019 10:53
@hsunpei hsunpei self-assigned this Apr 26, 2019
@hsunpei hsunpei added breaking and removed breaking labels Apr 26, 2019
// ----------------------
// Inner image
// ----------------------
> img {
Copy link
Contributor

Choose a reason for hiding this comment

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

You probably need a & here?

Suggested change
> img {
& > img {

* @param {array} fromOptions
* @return {Map}
*/
function getValueAvatarMap(fromChildren = []) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can this be merged with getValueLabelMap so we don't need to iterate the Array twice, only for deriving different data?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nice suggestion. Thanks!

import DebugBox from 'utils/DebugBox';

function BasicCheckboxExample() {
const rdAvatar = <Avatar type="square" alt="Avatar of RD" src="https://api.adorable.io/avatars/285/rd@ichef.tw" />;
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we change the email here to more generic one, maybe johndoe@example.com?

@@ -17,6 +17,7 @@ export const TYPE_SYMBOL = Symbol('SelectOption');
function SelectOption({
label,
value,
avatar,
Copy link
Contributor

Choose a reason for hiding this comment

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

missing type check here?

@hsunpei
Copy link
Contributor Author

hsunpei commented Apr 30, 2019

@zhusee2 The code has been refined. You may take a look at it when you're free 😃

Copy link
Contributor

@zhusee2 zhusee2 left a comment

Choose a reason for hiding this comment

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

LGTM 🥇

@hsunpei hsunpei merged commit e323dd2 into develop May 3, 2019
@zhusee2 zhusee2 added this to the 3.0.x milestone May 9, 2019
@zhusee2 zhusee2 mentioned this pull request May 9, 2019
@zhusee2 zhusee2 deleted the feature/avatar branch August 26, 2019 03:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants