breaking(Progress): control progress solely with progress #1355

BrainMaestro · 2017-02-20T16:40:28Z

Breaking Changes!

The label prop no longer accepts ratio|percent as a means to control the progress readout on the bar. It now refers to the text label applied to the element. The children prop takes precedence however.
The progress prop now handles this by either accepting a boolean which defaults to percent or a string with either ratio or percent.

Before

<Progress total={2} value={1} label='ratio' />
<Progress total={2} value={1} label='percent' />

After

<Progress total={2} value={1} progress='ratio' />
<Progress total={2} value={1} progress='percent' />
<Progress total={2} value={2} label='Dragon ISS Berthing' />

Unchanged

<Progress total={2} value={2}>Dragon ISS Berthing</Progress>

Fixes #1352

layershifter · 2017-02-20T16:54:26Z

src/modules/Progress/Progress.js

@@ -140,10 +140,9 @@ class Progress extends Component {
  }

  showProgress = () => {
-    const { label, precision, progress, total, value } = this.props
+    const { label, precision, progress } = this.props

    if (label || progress || !_.isUndefined(precision)) return true


Let's make this condition more simple:

-if (label || progress || !_.isUndefined(precision)) return true +return label || progress || !_.isUndefined(precision)

Sure. I noticed it, but I didn't want to make changes that were not directly related to the task. I'll do that now

codecov-io · 2017-02-20T16:57:05Z

Codecov Report

Merging #1355 into master will not change coverage.
The diff coverage is 100%.

@@           Coverage Diff           @@
##           master    #1355   +/-   ##
=======================================
  Coverage   99.74%   99.74%           
=======================================
  Files         140      140           
  Lines        2346     2346           
=======================================
  Hits         2340     2340           
  Misses          6        6

Impacted Files	Coverage Δ
src/modules/Progress/Progress.js	`100% <100%> (ø)`	✅
src/modules/Dropdown/Dropdown.js	`100% <ø> (ø)`	✅
src/addons/TextArea/TextArea.js	`100% <ø> (ø)`	✅
src/collections/Form/Form.js	`100% <ø> (ø)`	✅
src/modules/Checkbox/Checkbox.js	`100% <ø> (ø)`	✅
src/views/Feed/Feed.js	`100% <ø> (ø)`	✅
src/modules/Modal/Modal.js	`100% <ø> (ø)`	✅

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 82cdbe9...5c3d43a. Read the comment docs.

layershifter

@BrainMaestro Thanks for PR 👍

levithomason · 2017-02-21T00:02:13Z

Issue

I think we need to rework the relationship of the progress and label props altogether.

SUI core offers a label setting, which takes a value of percent or ratio. This is where we got our prop label. However, this is super confusing since the label prop controls the progress readout, and the children control the actual label component.
It also makes for this very confusing case where the label boolean prop does exactly the same thing as the booleanprogress prop:

Fix Proposal

I think we need to make the progress prop control the progress readout and make the label prop control the label component.

This would be more inline with our prop-to-subcomponent relationship we have across the entire library as well.

Thoughts?

BrainMaestro · 2017-02-21T07:31:22Z

@levithomason I agree. That makes it clearer. The other way was slightly confusing.

levithomason · 2017-02-21T16:27:44Z

Awesome. Here are some pointers are how to handle the new propTypes and values. The label prop is now a "shorthand" prop used to create the label element. We should start with propTypes like so:

    /** Can be set to either to display progress as percent or ratio. */
    label: customPropTypes.itemShorthand,

    /** A progress bar can contain a text value indicating current progress. */
    progress: PropTypes.oneOfType([
      PropTypes.bool,
      PropTypes.oneOf(['ratio', 'percent']),
    ]),

The itemShorthand allows users to pass a number, string, props object, or an element from which we will then create an element for them. In this case we'll create the className='label' div.

We have a factory function for doing this called, createShorthand. You get it by adding it to the lib import:

import { createShorthand } from '../../lib'

This function takes an element class (string or function), a function to map number/string values to props, and finally the value you want to create an element with (number, string, props object, or another element). Create a labelElement in the render method (comments can be left out):

const labelElement = createShorthand(
  'div',                                // create a div
  val => ({ children: val }),           // map string/numbers to children
  label,                                // use the label prop as the value
  { className: 'label' }                // default props
)

Now, we can conditionally display the children or the labelElement, whichever the user passed.

-{children && <div className='label'>{children}</div>}
+{children ? <div className='label'>{children}</div> : labelElement}

BrainMaestro · 2017-02-22T13:55:21Z

Hey @levithomason. Thanks for the task breakdown. I implemented it exactly as you suggested, but the check {children ? fails for any falsy value which includes 0 that is used in the renders child number with 0 value test. Should I use !_.isUndefined in the render function which is how I solved the problem? Or do you have a cleaner approach?

levithomason · 2017-02-22T22:38:22Z

We've updated the code base to use _.isNil(children) so as to capture null and undefined.

BrainMaestro · 2017-02-23T13:46:41Z

Okay has it been merged in?

BrainMaestro · 2017-02-23T13:48:08Z

src/modules/Progress/Progress.js

            </div>
          )}
        </div>
-        {children && <div className='label'>{children}</div>}
+        {!_.isUndefined(children) ? <div className='label'>{children}</div> : labelElement}


This is the line I was talking about. Can I safely change it to {children ? <div className='label'>{children}</div> : labelElement} without the false positives of 0 failing the truthy check?

- {!_.isUndefined(children) ? <div className='label'>{children}</div> : labelElement} + {_.isNil(children) ? this.getLabel() : <div className='label'>{children}</div>}

okay the label should take priority over the children? I thought it was the reverse. I will make the change

{_.isNil(children) ? this.getLabel() : <div className='label'>{children}</div>} {!_.isNil(children) ? <div className='label'>{children}</div> : this.getLabel()}

There conditions do same, but first looks more clear to me.

layershifter · 2017-02-23T14:28:20Z

src/modules/Progress/Progress.js

            </div>
          )}
        </div>
-        {children && <div className='label'>{children}</div>}
+        {!_.isNil(children) ? <div className='label'>{children}</div> : this.getLabel()}


I think we can move this condition to getLabel:

- getLabel = () => { - const { label } = this.props - - return createShorthand( - 'div', - val => ({ children: val }), - label, - { className: 'label' } - ) - } + renderLabel = () => { + const { children, label } = this.props + + if(!_.isNil(children)) return <div className='label'>{children}</div> + return createShorthand( + 'div', + val => ({ children: val }), + label, + { className: 'label' } + ) + }

layershifter · 2017-02-23T14:37:49Z

src/modules/Progress/Progress.js

@@ -190,11 +195,11 @@ class Progress extends Component {
        <div className='bar' style={{ width: `${percent}%` }}>
          {this.showProgress() && (
            <div className='progress'>
-              { label !== 'ratio' ? `${percent}%` : `${value}/${total}` }
+              { progress !== 'ratio' ? `${percent}%` : `${value}/${total}` }
            </div>


And this to renderProgress:

- {this.showProgress() && ( - <div className='progress'> - { progress !== 'ratio' ? `${percent}%` : `${value}/${total}` } - </div> - }) + {this.renderProgress()}

+ renderProgress = () => { + const { precision, progress } = this.props + + if(!progress && _.isUndefined(precision)) return + return ( + <div className='progress'> + { progress === 'ratio' ? `${value}/${total}` : `${percent}%` } + </div> + ) +}

levithomason · 2017-02-23T23:21:29Z

Excellent!

levithomason · 2017-02-23T23:23:21Z

This is a breaking change, so we need to update the PR title and description to include upgrade notes. Reference #1367 or #989 for good examples.

…emantic-Org#1355) * fix(Progress): do not show progress without progress or label props * style(Progress): some style fixes * style(Progress): update typings * Update Progress.js

levithomason · 2017-03-01T06:33:34Z

Thanks for the great notes @BrainMaestro 👍

levithomason · 2017-03-01T07:36:43Z

Released in [email protected].

BrainMaestro force-pushed the fix-progress-bar branch from 2056b84 to 6a7a830 Compare February 20, 2017 16:53

layershifter requested changes Feb 20, 2017

View reviewed changes

BrainMaestro force-pushed the fix-progress-bar branch from 6a7a830 to b248a18 Compare February 20, 2017 16:55

layershifter approved these changes Feb 20, 2017

View reviewed changes

layershifter added the ready for review label Feb 20, 2017

layershifter mentioned this pull request Feb 20, 2017

docs(Progress): added example for progress label prop #1356

Merged

levithomason added needs author feedback and removed ready for review labels Feb 21, 2017

levithomason mentioned this pull request Feb 21, 2017

Progress bar issue in Meteor #1339

Closed

BrainMaestro force-pushed the fix-progress-bar branch from b248a18 to 341a036 Compare February 22, 2017 13:55

BrainMaestro commented Feb 23, 2017

View reviewed changes

BrainMaestro force-pushed the fix-progress-bar branch 3 times, most recently from 20957e3 to 206ca0f Compare February 23, 2017 14:22

layershifter requested changes Feb 23, 2017

View reviewed changes

fix(Progress): do not show progress without progress or label props

463b1f3

BrainMaestro force-pushed the fix-progress-bar branch from 206ca0f to 463b1f3 Compare February 23, 2017 15:01

layershifter added 3 commits February 23, 2017 17:10

style(Progress): some style fixes

4613f60

style(Progress): update typings

a6f15fc

Update Progress.js

5c3d43a

layershifter approved these changes Feb 23, 2017

View reviewed changes

layershifter added ready for review and removed needs author feedback labels Feb 23, 2017

levithomason merged commit 7387b4b into Semantic-Org:master Feb 23, 2017

BrainMaestro changed the title ~~fix(Progress): do not show progress without progress or label props~~ breaking(Progress): control progress solely with progress Feb 24, 2017

BrainMaestro deleted the fix-progress-bar branch February 24, 2017 08:38

levithomason mentioned this pull request Mar 1, 2017

docs(Progress): fix label/progress prop docs #1402

Merged

levithomason added the breaking-change label Dec 10, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

breaking(Progress): control progress solely with progress #1355

breaking(Progress): control progress solely with progress #1355

BrainMaestro commented Feb 20, 2017 •

edited

Loading

layershifter Feb 20, 2017

BrainMaestro Feb 20, 2017

codecov-io commented Feb 20, 2017 •

edited

Loading

layershifter left a comment

levithomason commented Feb 21, 2017

BrainMaestro commented Feb 21, 2017

levithomason commented Feb 21, 2017 •

edited

Loading

BrainMaestro commented Feb 22, 2017

levithomason commented Feb 22, 2017

BrainMaestro commented Feb 23, 2017

BrainMaestro Feb 23, 2017

layershifter Feb 23, 2017

BrainMaestro Feb 23, 2017

layershifter Feb 23, 2017 •

edited

Loading

layershifter Feb 23, 2017

layershifter Feb 23, 2017

levithomason commented Feb 23, 2017

levithomason commented Feb 23, 2017

levithomason commented Mar 1, 2017

levithomason commented Mar 1, 2017

breaking(Progress): control progress solely with progress #1355

breaking(Progress): control progress solely with progress #1355

Conversation

BrainMaestro commented Feb 20, 2017 • edited Loading

Breaking Changes!

Before

After

Unchanged

layershifter Feb 20, 2017

Choose a reason for hiding this comment

BrainMaestro Feb 20, 2017

Choose a reason for hiding this comment

codecov-io commented Feb 20, 2017 • edited Loading

Codecov Report

layershifter left a comment

Choose a reason for hiding this comment

levithomason commented Feb 21, 2017

Issue

Fix Proposal

BrainMaestro commented Feb 21, 2017

levithomason commented Feb 21, 2017 • edited Loading

BrainMaestro commented Feb 22, 2017

levithomason commented Feb 22, 2017

BrainMaestro commented Feb 23, 2017

BrainMaestro Feb 23, 2017

Choose a reason for hiding this comment

layershifter Feb 23, 2017

Choose a reason for hiding this comment

BrainMaestro Feb 23, 2017

Choose a reason for hiding this comment

layershifter Feb 23, 2017 • edited Loading

Choose a reason for hiding this comment

layershifter Feb 23, 2017

Choose a reason for hiding this comment

layershifter Feb 23, 2017

Choose a reason for hiding this comment

levithomason commented Feb 23, 2017

levithomason commented Feb 23, 2017

levithomason commented Mar 1, 2017

levithomason commented Mar 1, 2017

BrainMaestro commented Feb 20, 2017 •

edited

Loading

codecov-io commented Feb 20, 2017 •

edited

Loading

levithomason commented Feb 21, 2017 •

edited

Loading

layershifter Feb 23, 2017 •

edited

Loading