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.

Sign up for GitHub

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

Jump to bottom

feat: add Avatar component #1191

Merged
merged 1 commit into from
Jan 12, 2022
Merged

feat: add Avatar component #1191

merged 1 commit into from
Jan 12, 2022

Conversation

langz
Copy link
Contributor

@langz langz commented Jan 7, 2022 •
edited
Loading

Summary

Link to Figma: https://www.figma.com/file/cdtwQD8IJ7pTeE45U148r1/Eufemia---Web?node-id=17869%3A0

TODO in the future:

  • Incorporate "badges" (see Figma sketches):
    image

@langz langz marked this pull request as draft January 7, 2022 11:43
@codesandbox-ci
Copy link

codesandbox-ci bot commented Jan 7, 2022 •
edited
Loading

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit fd67ecb:

Sandbox Source
eufemia-starter Configuration

@gatsby-cloud
Copy link

gatsby-cloud bot commented Jan 7, 2022 •
edited
Loading

Gatsby Cloud Build Report

DNB Eufemia Portal

🎉 Your build was successful! See the Deploy preview here.

Build Details

View the build logs here.

🕐 Build time: 6m

Performance

Lighthouse report

Metric Score
Performance 🔶 65
Accessibility 💚 100
Best Practices 💚 100
SEO 💚 92

🔗 View full report

@langz langz force-pushed the feat/avatar-component branch 2 times, most recently from 820aa48 to b6c5422 Compare January 7, 2022 12:20
langz
langz commented Jan 7, 2022
View reviewed changes
Copy link
Contributor Author

@langz langz left a comment

Choose a reason for hiding this comment

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

To me it looks okay, just have to verify the sizes with what's used in pm-netbank today.

@langz langz force-pushed the feat/avatar-component branch 2 times, most recently from 96dd19f to a1ef3b4 Compare January 10, 2022 22:13
@langz langz marked this pull request as ready for review January 10, 2022 22:13
thaytharma
thaytharma reviewed Jan 11, 2022
View reviewed changes
packages/dnb-design-system-portal/src/docs/uilib/components/avatar/properties.md Outdated

| Properties | Description |
| ------------------------------------------- | --------------------------------------------------------------------------------------------------------------------------------- |
| `size` | _(optional)_ Size of the avatar. Options: `small` \| `medium` \| `large` \| `x-large`. |
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggestion:

It seems like the default-size is the same size as the medium. Should we consider removing default and use medium as default value instead? Also, I think it's beneficial to update the description here with the default size.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Correct, the default/fallback size when not providing any size is medium.
As far as I see it, the value default is not supported for the size property. So "removing default" shouldn't be needed/possible.

I'll add a description/text explaining that the default value of size is medium ✅

Copy link
Contributor

@thaytharma thaytharma Jan 11, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Perfect!

thaytharma
thaytharma reviewed Jan 11, 2022
View reviewed changes
packages/dnb-design-system-portal/src/docs/uilib/components/avatar/properties.md Outdated
| `src` | _(optional)_ Specifies the path to the image |
| `skeleton` | _(optional)_ Applies loading skeleton. |
| `imgProps` | _(optional)_ [Image properties](/uilib/elements/image) applied to the `img` element if the component is used to display an image. |
| `variant` | _(optional)_ Override the variant of the component. Options: `primary` \| `secondary` \| `tertiary`. |
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
| `variant` | _(optional)_ Override the variant of the component. Options: `primary` \| `secondary` \| `tertiary`. |
| `variant` | _(optional)_ Override the variant of the component. Options: `primary` \| `secondary` \| `tertiary`. Default to `primary`. |

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 find, I'll add that 🙇

@thaytharma
Copy link
Contributor

Looks great! 🤩

@langz langz force-pushed the feat/avatar-component branch 3 times, most recently from a213600 to 282b998 Compare January 11, 2022 11:46
tujoworker
tujoworker approved these changes Jan 11, 2022
View reviewed changes
Copy link
Member

@tujoworker tujoworker left a comment

Choose a reason for hiding this comment

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

Yes, indeed! Awesome work 🤩

@tujoworker
Copy link
Member

The only thing we need to improve is the requirement for additional, screen reader information. E.g. to require with a prop more info?

@langz langz force-pushed the feat/avatar-component branch from 282b998 to fd67ecb Compare January 11, 2022 15:00
@langz
Copy link
Contributor Author

langz commented Jan 11, 2022 •
edited
Loading

The only thing we need to improve is the requirement for additional, screen reader information. E.g. to require with a prop more info?

@tujoworker, I am not sure if I fully understood this.
Could you try to elaborate in a bit more details? Which attributes/properties are we thinking of, etc? Do we have an example of this in any of our other components?
Then I could try to implement this improvement.

@langz langz merged commit 6a30523 into main Jan 12, 2022
@langz langz deleted the feat/avatar-component branch January 12, 2022 20:11
tujoworker pushed a commit that referenced this pull request Jan 26, 2022
@semantic-release-bot
chore(release): 9.19.0 [skip ci]
6e58d85 
# [9.19.0](v9.18.0...v9.19.0) (2022-01-26)

### Bug Fixes

* add "unstyled" as a button variant ([8cb0712](8cb0712))
* added export of Bar in Pagination ([#1200](#1200)) ([f148786](f148786))
* **Avatar:** improve accessibility ([#1213](#1213)) ([3a456c5](3a456c5))
* **Breadcrumb:** forward props in BreadcrumbItem to span as well ([#1188](#1188)) ([17a1de0](17a1de0))
* **Breadcrumb:** improve accessibility ([#1189](#1189)) ([59825b8](59825b8))
* **build:** break build when step fails ([cb436d3](cb436d3))
* **CodeHighlighting:** ensure a scrollbar is shown when not enough space is available ([634df1b](634df1b))
* **DrawerList:** ensure DrawerList is unsetting its global attributes on unmount ([d64ccf8](d64ccf8))
* fix circular dependency issue for Timeline and Avatar ([439ccc3](439ccc3))
* fix rendering of DatePickerFooter when show_reset_button is provided ([#1197](#1197)) ([d7d4507](d7d4507))
* fix reset_button_text property of DatePicker ([#1196](#1196)) ([e50fb6f](e50fb6f))
* fixed global context bug that removed component props ([#1193](#1193)) ([82be98c](82be98c))
* **Tag:** fix circular dependency issue when bundling UMD modules with Rollup.js ([d155f7c](d155f7c))
* **Tag:** remove data-prop from TagGroup ([#1225](#1225)) ([72e4207](72e4207))
* **Tag:** remove tag group warnings in tests ([#1214](#1214)) ([16bd840](16bd840))
* **Tag:** update delete icon ([#1224](#1224)) ([fe5c766](fe5c766))

### Features

* add Avatar component ([#1191](#1191)) ([6a30523](6a30523))
* add AvatarGroup component ([#1195](#1195)) ([ae5ef7d](ae5ef7d))
* add InfoCard component ([5db0cbc](5db0cbc))
* add TagGroup component ([#1208](#1208)) ([781007d](781007d))
* add TimeLine component ([#1186](#1186)) ([255a14e](255a14e))
* remove type="module" in order omit the "fullySpecified" spec ([b424e08](b424e08))
* **Tag:** Add onClick prop ([db68b30](db68b30))
* **Tag:** add onDelete-prop ([#1217](#1217)) ([2f1642b](2f1642b))
@tujoworker
Copy link
Member

🎉 This PR is included in version 9.19.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@thaytharma thaytharma thaytharma left review comments

@tujoworker tujoworker tujoworker approved these changes

Assignees
No one assigned
Labels
released
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@langz @thaytharma @tujoworker