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

feat(module:skeleton):add skeleton component #1829

Merged
merged 2 commits into from Sep 17, 2018

Conversation

Projects
None yet
3 participants
@wenqi73
Contributor

wenqi73 commented Jul 15, 2018

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

[ ] Bugfix
[x] Feature
[ ] Code style update (formatting, local variables)
[ ] Refactoring (no functional changes, no api changes)
[ ] Build related changes
[ ] CI related changes
[ ] Documentation content changes
[ ] Application (the showcase website) / infrastructure changes
[ ] Other... Please describe:

What is the current behavior?

Issue Number: #1808

What is the new behavior?

Does this PR introduce a breaking change?

[ ] Yes
[x] No

Other information

@wenqi73 wenqi73 force-pushed the wenqi73:fea-skeleton branch from 00877cf to b2734ec Jul 15, 2018

@codecov

This comment has been minimized.

codecov bot commented Jul 15, 2018

Codecov Report

Merging #1829 into master will increase coverage by <.01%.
The diff coverage is 98.55%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1829      +/-   ##
==========================================
+ Coverage      96%   96.01%   +<.01%     
==========================================
  Files         477      479       +2     
  Lines       11618    11685      +67     
  Branches     1550     1561      +11     
==========================================
+ Hits        11154    11219      +65     
  Misses        132      132              
- Partials      332      334       +2
Impacted Files Coverage Δ
components/card/demo/loading.ts 100% <ø> (+16.66%) ⬆️
components/skeleton/nz-skeleton.module.ts 100% <100%> (ø)
components/skeleton/nz-skeleton.component.ts 98.46% <98.46%> (ø)
components/tabs/nz-tabset.component.ts 95.72% <0%> (-0.86%) ⬇️
components/table/nz-table.component.ts 95.34% <0%> (-0.59%) ⬇️

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 6ef1185...c201b4b. Read the comment docs.

@wenqi73 wenqi73 force-pushed the wenqi73:fea-skeleton branch from b2734ec to 3a4d02c Jul 15, 2018

@HsuanXyz HsuanXyz requested a review from wendzhue Jul 16, 2018

</span>
</div>
<div class="ant-skeleton-content">
<h3 *ngIf="!!nzTitle" class="ant-skeleton-title" [style.width]="toCSSUnit(_title.width)"></h3>

This comment has been minimized.

@wendzhue

wendzhue Jul 16, 2018

Member

It's not a good idea to access a private variable in template files. It may cause errors in --aot compiling.

return {};
}
toCSSUnit(value: number | string = ''): number | string {

This comment has been minimized.

@wendzhue

wendzhue Jul 16, 2018

Member

It seems that this function would not return a number.

- 网络较慢,需要长时间等待加载处理的情况下
- 图文信息内容较多的列表/卡片中

This comment has been minimized.

@wendzhue

wendzhue Jul 16, 2018

Member

Could you please add the other demo? See: https://deploy-preview-11226--ant-design.netlify.com/components/skeleton-cn/.

It would be good if we sync to ant design react.

This comment has been minimized.

@wenqi73

wenqi73 Jul 16, 2018

Contributor

thx, I just sent a commit

@wenqi73 wenqi73 force-pushed the wenqi73:fea-skeleton branch from 2070fe2 to 622f9b8 Jul 16, 2018

@wendzhue

This comment has been minimized.

Member

wendzhue commented Jul 16, 2018

@wenqi73 Thanks for you contribution. Hopefully we would commit these to master branch this weekend.

@wenqi73 wenqi73 force-pushed the wenqi73:fea-skeleton branch 2 times, most recently from 9ecea6e to d43346e Jul 17, 2018

@wenqi73 wenqi73 changed the title from feat(module:skeleton):add skeleton component to WIP(module:skeleton):add skeleton component Jul 23, 2018

@wenqi73 wenqi73 force-pushed the wenqi73:fea-skeleton branch 3 times, most recently from 39e6d3b to e535dd0 Aug 3, 2018

@wenqi73 wenqi73 force-pushed the wenqi73:fea-skeleton branch from e535dd0 to d2c0eec Aug 23, 2018

@wenqi73

This comment has been minimized.

Contributor

wenqi73 commented Aug 23, 2018

@wendzhue Please review again.

@wendzhue

wendzhue requested changes Aug 23, 2018 edited

Your code is of good quality with minor changes requested. We would merge this after Ant Design officially releases this component and make some changes accordingly.

}
updateProps(): void {
this.title = {

This comment has been minimized.

@wendzhue

wendzhue Aug 23, 2018

Member

I know you're trying to cover default props with user's customized props. But these ... operators seems verbose to me. Could you figure out a simpler and direct way to prepare these parameters?

@Input() nzAvatar: NzSkeletonAvatar | boolean = false;
@Input() nzParagraph: NzSkeletonParagraph | boolean = true;
getTitleBasicProps(hasAvatar: boolean, hasParagraph: boolean): NzSkeletonTitle {

This comment has been minimized.

@wendzhue

wendzhue Aug 23, 2018

Member

You don't need to expose these get* methods as public.

There are some other methods should be set as private too.

You'd better write them into updateProps since they're only used there, to improve code clarity. And in that way you don't have to pass these parameters all over the place.

}
getAvatarBasicProps(hasTitle: boolean, hasParagraph: boolean): NzSkeletonAvatar {
if (hasTitle && !hasParagraph) {

This comment has been minimized.

@wendzhue

wendzhue Aug 23, 2018

Member

Use ?: operator for code readability. Like:

const shape = hasTitle && !hasParagraph ? 'square' : 'circle';
return { shape };
toCSSUnit(value: number | string = ''): string {
if (typeof value === 'number') {
return `${value}px`;
} else if (typeof value === 'string') {

This comment has been minimized.

@wendzhue

wendzhue Aug 23, 2018

Member

typeof '' returns "string". It's not reliable though it works because when width is not truthy, the line goes 100% wide as a block element.

This comment has been minimized.

@wenqi73

wenqi73 Sep 8, 2018

Contributor

I revert this because the width is controlled by less:
https://github.com/ant-design/ant-design/blob/02af08951ad4b0a461fc282382f3c12dd5b6d7d1/components/skeleton/style/index.less#L63-L65
width = '' will not affect default width, maybe there is a better solution.

widthList = width;
} else if (width && !Array.isArray(width)) {
widthList = [];
widthList[rows - 1] = width;

This comment has been minimized.

@wendzhue

wendzhue Aug 23, 2018

Member

I know it would work silently in situations like [undefined, undefined, 80] and make the first two lines' width property not set. But worrisome...

No need to change perhaps.

@wendzhue

This comment has been minimized.

Member

wendzhue commented Aug 23, 2018

@wenqi73 And there's an avatar skeleton demo in card component page. Please add it. See https://deploy-preview-11226--ant-design.netlify.com/components/card-cn/#components-card-demo-loading.

Besides, please change your first commit message to feat(module:skeleton): add skeleton component once you're all done.

@wenqi73 wenqi73 force-pushed the wenqi73:fea-skeleton branch from 8f624e3 to 24d3320 Aug 26, 2018

@wenqi73

This comment has been minimized.

Contributor

wenqi73 commented Aug 26, 2018

@wendzhue Another question: card loading demo has changed to skeleton, I think the test is useless so I commented it. please check again and I will change the commit message later.

@wenqi73 wenqi73 force-pushed the wenqi73:fea-skeleton branch from 24d3320 to d1754ac Aug 26, 2018

@wenqi73 wenqi73 changed the title from WIP(module:skeleton):add skeleton component to feat(module:skeleton):add skeleton component Aug 26, 2018

@wendzhue

This comment has been minimized.

Member

wendzhue commented Sep 5, 2018

@wenqi73 Ant Design had released this component and added a new demo. See https://ant.design/components/skeleton-cn/#components-skeleton-demo-complex. Please add it.

@wendzhue

This comment has been minimized.

Member

wendzhue commented Sep 5, 2018

And please rebase your commit to tag 1.4.1. Since it was checked out some versions ago.

@wendzhue

This comment has been minimized.

Member

wendzhue commented Sep 5, 2018

And nz-card's demo should not be removed (neither its test). In fact it should support two ways of usage at the same time. See https://ant.design/components/card-cn/#components-card-demo-loading.

@wenqi73 wenqi73 force-pushed the wenqi73:fea-skeleton branch from 2bcc679 to c201b4b Sep 8, 2018

@wenqi73

This comment has been minimized.

Contributor

wenqi73 commented Sep 8, 2018

@wendzhue demo has added, please check.

@wendzhue

This comment has been minimized.

Member

wendzhue commented Sep 8, 2018

LGTM. @simplejason If possible, this could be released this weekend.

@vthinkxie vthinkxie merged commit fd34f8b into NG-ZORRO:master Sep 17, 2018

4 checks passed

codecov/patch 98.55% of diff hit (target 96%)
Details
codecov/project 96.01% (+<.01%) compared to 6ef1185
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
deploy/netlify Deploy preview ready!
Details

@wenqi73 wenqi73 deleted the wenqi73:fea-skeleton branch Sep 28, 2018

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