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

fix(Avatar): Allow Avatar.Group to set skeleton prop #1350

Merged
merged 3 commits into from Mar 22, 2022

Conversation

langz
Copy link
Contributor

@langz langz commented Mar 10, 2022

This PR just adds the property and type for skeleton to the Avatar.Group, so that it will be easier/possible to use the component with the skeleton prop. The functionality is/was already in place, take a look at https://eufemia.dnb.no/uilib/components/avatar and add the skeleton property to one of the demos/examples.

<Avatar.Group skeleton label="label">
   <Avatar>A</Avatar>
</Avatar.Group>

@codesandbox-ci
Copy link

codesandbox-ci bot commented Mar 10, 2022

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 515e6f6:

Sandbox Source
eufemia-starter Configuration

@gatsby-cloud
Copy link

gatsby-cloud bot commented Mar 10, 2022

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: 9m

Performance

Lighthouse report

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

🔗 View full report

* Skeleton should be applied when loading content
* Default: null
*/
skeleton?: SkeletonShow
Copy link
Contributor Author

@langz langz Mar 11, 2022

Choose a reason for hiding this comment

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

Not sure if/how I should use this prop in the component.
As it doesn't seem like I actually need to make more code changes to make the following code behave as expected:

<Avatar.Group skeleton label="label">
   <Avatar>A</Avatar>
</Avatar.Group>

@langz langz force-pushed the fix/allow-avatar-group-to-set-skeleton-prop branch from 6e43569 to eda9362 Compare March 11, 2022 08:47
@@ -55,6 +62,7 @@ export const defaultProps = {
size: 'medium',
children: null,
variant: 'primary',
skeleton: null,
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
skeleton: null,
skeleton: false,

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done ✅

@dinarosv dinarosv force-pushed the fix/allow-avatar-group-to-set-skeleton-prop branch from 91cd141 to cc43473 Compare March 14, 2022 14:00
@langz langz force-pushed the fix/allow-avatar-group-to-set-skeleton-prop branch from 0e867e3 to 9ad195d Compare March 14, 2022 19:08
@langz
Copy link
Contributor Author

langz commented Mar 14, 2022

Not really sure what I/we've done now, when running eufemia locally, I get the following errors in the browsers console when visiting http://localhost:8000/uilib/components/avatar/demos

Warning: Received `false` for a non-boolean attribute `skeleton`.
If you want to write it to the DOM, pass a string instead: skeleton="false" or skeleton={value.toString()}.
If you used to conditionally omit it with skeleton={condition && value}, pass skeleton={condition ? value : undefined} instead.

I'll try looking into it tomorrow.

@langz
Copy link
Contributor Author

langz commented Mar 15, 2022

What I wanted to achieve with this PR was the following:
image

At the moment, it's not working when passing skeleton to Avatar.Group, and I am not exactly sure why, but it seems to "magically work again" if I remove the following line #1347 (comment)

@dinarosv
Copy link
Contributor

dinarosv commented Mar 15, 2022

At the moment, it's not working when passing skeleton to Avatar.Group, and I am not exactly sure why, but it seems to "magically work again" if I remove the following line #1347 (comment)

I think you might need to pass down skeleton from AvatarGroup to the clone function for the Avatar? Or another option is to include it in the AvatarGroup context .... edit: what if you change the order of {skeleton: context?.skeleton} and avatargroupcontext in Avatar?

@langz langz force-pushed the fix/allow-avatar-group-to-set-skeleton-prop branch from 9ad195d to 515e6f6 Compare March 22, 2022 09:43
@langz
Copy link
Contributor Author

langz commented Mar 22, 2022

At the moment, it's not working when passing skeleton to Avatar.Group, and I am not exactly sure why, but it seems to "magically work again" if I remove the following line #1347 (comment)

I think you might need to pass down skeleton from AvatarGroup to the clone function for the Avatar? Or another option is to include it in the AvatarGroup context .... edit: what if you change the order of {skeleton: context?.skeleton} and avatargroupcontext in Avatar?

Thanks, that did it 🙇 🙏

@langz langz merged commit a0d4e8a into main Mar 22, 2022
@langz langz deleted the fix/allow-avatar-group-to-set-skeleton-prop branch March 22, 2022 10:10
tujoworker pushed a commit that referenced this pull request Apr 5, 2022
# [9.25.0](v9.24.0...v9.25.0) (2022-04-05)

### Bug Fixes

* **Anchor:** Missing properties in PropTypes ([#1382](#1382)) ([92b6615](92b6615))
* **Avatar:** Allow Avatar to get skeleton prop from context ([#1347](#1347)) ([386616b](386616b))
* **Avatar:** Allow Avatar.Group to set skeleton prop ([#1350](#1350)) ([a0d4e8a](a0d4e8a))
* **Breadcrumb:** Allow Breadcrumb to get skeleton prop from context ([#1349](#1349)) ([b46f83b](b46f83b))
* **Button:** make tertiary button icon inherit button height property ([04111d5](04111d5))
* **Button:** remove outline when button goes from enabled to disabled state when activeElement ([8f40076](8f40076))
* declare v17 as Eufemias peer dependency ([b14d237](b14d237))
* **Dialog:** prevent close confirmation dialog on outside click ([d391285](d391285))
* **Dropdown:** ensure the ellipsis text overflow works again ([640bdf9](640bdf9))
* **FormStatus:** wider the min-width to 30rem and react on window width change ([adb2f3f](adb2f3f))
* **Hr:** ensure the hr element does not causing a scrollbar ([cca94ff](cca94ff))
* **InfoCard:** Allow InfoCard to get skeleton prop from context ([#1348](#1348)) ([e2290c2](e2290c2))
* **Modal:** containerPlacement bug for top ([5864004](5864004))
* **Pagination:** add better experience for small viewports ([28ad651](28ad651))
* **Pagination:** handle unknown / undefined page_count ([acc9c81](acc9c81))
* **Pagination:** updated media query calculation ([35344ea](35344ea))
* **Pagination:** use primary-icon instead of secondary import ([6944863](6944863))
* remove alignment helpers from a11y tree (NVDA & Firefox) ([f31be33](f31be33))
* **Skeleton:** Missing properties in PropTypes ([#1381](#1381)) ([9abc790](9abc790))
* **Tag:** Allow Tag to get skeleton prop from context ([3723421](3723421))
* **Tag:** Allow Tag.Group to set skeleton prop ([#1351](#1351)) ([844e215](844e215))
* **Tag:** avoid inheriting spacing props when set on Tag.Group ([fbb7ad1](fbb7ad1))
* **Theming:** simplify the theming logic ([9b50082](9b50082))
* **Timeline:** Allow Timeline to get skeleton prop from context ([#1346](#1346)) ([b87a59a](b87a59a))
* **Tooltip:** ensure portal element does not occupy space in body ([4b05280](4b05280))
* **Tooltip:** fix premature removal of portal and prevent zombies ([fb0fe33](fb0fe33))
* **Typography:** fix lead element and update docs ([3274dfe](3274dfe))
* **VisuallyHidden:** use span as default element so it can be used inside paragraphs ([8dbc8a3](8dbc8a3))

### Features

* add useNumberFormat hook ([b218d27](b218d27))
* **Drawer:** less spacing in new styles ([c4faaf2](c4faaf2))
* **Eiendom:** apply theme styles ([447ccd6](447ccd6))
* **helpers:** add cancel method to debounce ([19cb3e0](19cb3e0))
* **Icons:** add account_percent, news, podcast, user_feedback ([8695f57](8695f57))
* **Icons:** rebuild all icons from refactored Figma lib source ([fc952bb](fc952bb))
* **Modal:** add property omitTriggerButton for hiding default trigger ([26e90fb](26e90fb))
* **Modal:** add property omitTriggerButton for hiding default trigger ([d02bbdd](d02bbdd))
* **Pagination:** enhance accessibility and align styles to UX ([6fdba78](6fdba78))
* **Pagination:** moved next buttons in new design ([d516264](d516264))
* **Portal:** Suffix page titles with "| Eufemia" ([#1333](#1333)) ([788a58b](788a58b))
* **Theme:** add visual test example for Eiendom theme ([dcf918b](dcf918b))
* **Theme:** create Eufemia Theme handler (Gatsby Plugin) ([d257a55](d257a55))
@tujoworker
Copy link
Member

🎉 This PR is included in version 9.25.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
Labels
Development

Successfully merging this pull request may close these issues.

None yet

3 participants