feat(module:progress): add indeterminate state - #9677
Conversation
Summary of ChangesHello @Hbib24, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a new Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces an indeterminate state for the line progress component, which is a valuable feature. The implementation, including documentation and demos, is well-executed. I've provided a few suggestions to enhance API consistency, improve test coverage, and refine the CSS for better maintainability.
| // Should only work with line type | ||
| const progressBar = progress.nativeElement.querySelector('.ant-progress-bg'); | ||
| expect(progressBar).toBeTruthy(); | ||
| }); |
There was a problem hiding this comment.
The test correctly verifies the positive case for nzType="line". To make it more robust and prevent regressions, please also test the negative case: that ant-progress-indeterminate class is not applied when nzIndeterminate is true but nzType is not 'line' (e.g., 'circle').
You could add this at the end of the test:
// Should not work with other types
testComponent.type = 'circle';
fixture.detectChanges();
expect(progress.nativeElement.firstElementChild!.classList).not.toContain('ant-progress-indeterminate');| position: relative; | ||
| width: 100% !important; | ||
| overflow: hidden; |
There was a problem hiding this comment.
The width: 100% !important; rule is redundant. The component's template already sets the width to 100% via an inline style when nzIndeterminate is true. Since inline styles have higher precedence, this CSS rule is unnecessary. Removing it cleans up the code and avoids using !important, which is a good practice.
position: relative;
overflow: hidden;
There was a problem hiding this comment.
Pull request overview
This PR adds an indeterminate state feature to the progress component, allowing developers to display a continuous loading animation when the exact progress of a task cannot be determined. The feature is only available for line-type progress bars (nzType="line").
Changes:
- Added
nzIndeterminateboolean input property to control the indeterminate state - Implemented CSS animation with diagonal stripe pattern that moves continuously
- Added comprehensive tests for the new feature
- Added demo component showcasing various states (default, exception, success, without info)
- Updated documentation in both English and Chinese
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| components/progress/progress.component.ts | Added nzIndeterminate input property and conditional logic to set width to 100% and apply CSS class when indeterminate |
| components/progress/style/index.less | Added .ant-progress-indeterminate styles with diagonal stripe pattern and animation keyframes |
| components/progress/progress.spec.ts | Added test case to verify indeterminate class is applied correctly |
| components/progress/doc/index.zh-CN.md | Added Chinese documentation for the new nzIndeterminate property |
| components/progress/doc/index.en-US.md | Added English documentation for the new nzIndeterminate property |
| components/progress/demo/indeterminate.ts | Created demo component showing indeterminate progress in different states |
| components/progress/demo/indeterminate.md | Added demo documentation in both Chinese and English |
| it('should indeterminate work', () => { | ||
| fixture.detectChanges(); | ||
| expect(progress.nativeElement.firstElementChild!.classList).not.toContain('ant-progress-indeterminate'); | ||
|
|
||
| testComponent.indeterminate = true; | ||
| fixture.detectChanges(); | ||
| expect(progress.nativeElement.firstElementChild!.classList).toContain('ant-progress-indeterminate'); | ||
|
|
||
| // Should only work with line type | ||
| const progressBar = progress.nativeElement.querySelector('.ant-progress-bg'); | ||
| expect(progressBar).toBeTruthy(); | ||
| }); |
There was a problem hiding this comment.
The test could be more comprehensive. Consider adding assertions to verify: 1) that the progress bar width is set to 100% when indeterminate is true (the template sets [style.width.%]="nzIndeterminate ? 100 : nzPercent"), and 2) that the indeterminate state doesn't apply to circle or dashboard types by testing with those types.
| transform: translateX(0); | ||
| } | ||
|
|
||
| 100% { |
There was a problem hiding this comment.
The animation translate value of -22.627px is a magic number that could benefit from a comment explaining its derivation. This value represents the diagonal length of the 16px stripe pattern (16 * √2 ≈ 22.627px), which ensures the animation loops seamlessly with the repeating diagonal gradient. Adding a comment would improve maintainability.
| 100% { | |
| 100% { | |
| // 22.627px ≈ 16 * √2: move by one diagonal of the 16px diagonal stripe pattern | |
| // to ensure the indeterminate animation loops seamlessly with the repeating gradient. |
| @Component({ | ||
| selector: 'nz-demo-progress-indeterminate', | ||
| imports: [NzFlexModule, NzProgressModule], |
There was a problem hiding this comment.
According to the coding guidelines, all components should set changeDetection: ChangeDetectionStrategy.OnPush in the @Component decorator. This is missing from this demo component but is present in all other demo components in the codebase. Adding this will improve performance by reducing unnecessary change detection cycles.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #9677 +/- ##
=======================================
Coverage 90.09% 90.10%
=======================================
Files 574 574
Lines 23707 23709 +2
Branches 4816 4817 +1
=======================================
+ Hits 21360 21362 +2
Misses 1523 1523
Partials 824 824 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
Hi @Hbib24 , thanks for your nice contribution. As you know, we follow Ant Design's design principles and specifications. |
|
Ant Design seems to favor the spinner for this use case. ant-design/ant-design#31295 @Laffery however, this seems like a pretty useful functionality. Additional styles required to get the indeterminate state could live in patch LESS files, which IIRC are not the one synchronized with Ant. |
PR Checklist
Please check if your PR fulfills the following requirements:
PR Type
What kind of change does this PR introduce?
What is the current behavior?
Progress component is missing a state where the progress is indeterminate, in some cases, exact progress of certain tasks cannot be known until they are done.
Issue Number: #9679
What is the new behavior?
Progress component now introduces a new prop
nzIndeterminate="true"which makes the progress appear indefinite with an appropriate animation, note that it's only availalbe for progress withnzType="line"Does this PR introduce a breaking change?
Other information