Adding indeterminate ProgressIndicator variation #3679
Conversation
kristoferbrown
requested review from
betrue-final-final,
dzearing,
erichdev and
gokunymbus
|
LOVE this. |
|
ooo shiny!! |
| aria-valuetext={undefined} | ||
| className="ms-ProgressIndicator-progressBar" | ||
| role="progressbar" | ||
| style={ |
There was a problem hiding this comment.
Maybe add another test to ProgressIndicator.test.tsx so we have both determinate and indeterminate
There was a problem hiding this comment.
@lynamemi, good call. I've added a test so that there is one with a % complete and one without.
| } | ||
|
|
||
| percentComplete = Math.min(100, Math.max(0, percentComplete! * 100)); | ||
| this.props.percentComplete !== undefined ? percentComplete = Math.min(100, Math.max(0, percentComplete! * 100)) : percentComplete = 0; |
There was a problem hiding this comment.
This should be written like the following.
percentComplete = (this.props.percentComplete) ? Math.min(100, Math.max(0, percentComplete! * 100)) : 0;
There was a problem hiding this comment.
@battletoilet, The way you wrote this definitely makes more sense, but ultimately I had to change this line and a few other spots so that it would still show the % version if you give 0 for the % and then only show the indeterminate version if it's actually undefined. Let me know if this is still too messy.
| background-color: WindowText; | ||
| } | ||
| } | ||
| [dir='rtl'] .progressBar { |
There was a problem hiding this comment.
We actually have a mixin in _i18n.scss that we normally use for this scenario.
@include RTL { .progressBar { ... } }
Or something similar
| describe('ProgressIndicator', () => { | ||
| it('renders ProgressIndicator correctly', () => { | ||
| const component = renderer.create(<ProgressIndicator percentComplete={ 0.75 } />); | ||
| let tree = component.toJSON(); |
There was a problem hiding this comment.
This looks like it could be a const as well unless tree is mutated further in the process chain.
| width: 0; | ||
|
|
||
| &.indeterminate { | ||
| position: absolute; |
There was a problem hiding this comment.
position: absolute is defined twice, not needed in both line 54 and 59
There was a problem hiding this comment.
Also recommend avoiding nesting. It creates very specific rules that are 2 levels deep, which creates problems later if it ever is overridden.
* Basic indeterminate progress implementation * Fixing snapshot issues. * Adding zero check, prop comments, and removing unused SASS. * Adding RTL support. * Rush change * no message * Fixing build * Addressing PR comments and improving zero handling * Fixing snapshot
|
Is there any simple way to customize Indeterminate progress bar to make more width and less speed? |
6 participants
Pull request checklist
$ npm run changeDescription of changes
Adds a ProgressIndicator variant to indicate indeterminate or unknown progress. If no value is passed for the percentComplete prop, the indeterminate variant will be shown instead of the normal percentage variant.
Low quality .gif preview:
