feat(Labeled progress): allow consumers to pass additional label for progressbar #822

karelhala · 2018-10-20T22:09:39Z

What:
To make it possible for consumers to show what indicates current progress we have to allow them pass additional label. This PR addresses such issue by adding new prop label to progress component.

coveralls · 2018-10-20T22:19:01Z

Pull Request Test Coverage Report for Build 2796

3 of 3 (100.0%) changed or added relevant lines in 3 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage increased (+0.007%) to 81.154%

Totals
Change from base Build 2792:	0.007%
Covered Lines:	3420
Relevant Lines:	3973

💛 - Coveralls

dlabrecq · 2018-10-21T05:01:25Z

packages/patternfly-4/react-core/src/components/Progress/ProgressContainer.js

  const StatusIcon = variantToIcon.hasOwnProperty(variant) && variantToIcon[variant];
  return (
    <Fragment>
      <div className={css(styles.progressDescription)} id={`${parentId}-description`}>
        {title}
      </div>
      <div className={css(styles.progressStatus)}>
+        {/* TODO: add padding to label */}
+        {label}
        {(measureLocation === ProgressMeasureLocation.top || measureLocation === ProgressMeasureLocation.outside) && (
          <span className={css(styles.progressMeasure)}>{value}%</span>
        )}


I'm looking to wrap the percentage in parentheses like the snapshot below. It looks like I can only add text before it?

I'd prefer if the label was wrapped in the span so we get the same text style provided by progressMeasure. Perhaps we can use the value by default, if label has not been provided? I'm already providing the value, so I can easily handle my own label.

Your example fits this as well, you'll just use measureLocation={ProgressMeasureLocation.none} and pass your own label which will be whatever you'll want (string, node, number). If we'd were to force parentheses we'll be targeting only this specific example. But what if someone would want to show Already donwloaded: 50 %? We'd have to have another branching here.

But you are correct that I should add styles applied to progress number on top of those custom styles (so label has some padding).

Not asking to force parentheses. I just want the ability to provide my own label. If measureLocation, will hide it, that's fine. Just need the styling.

With this approach you'll be able to pass your own label. I think we don't understand each other on here. Check the example where you can pass whatever you want

<Progress value={33} title="Descriptive text here" label={<div>$106.30 (82.49%)</div>} measureLocation={ProgressMeasureLocation.none} />;

Or just plain old string

<Progress value={33} title="Descriptive text here" label="$106.30 (82.49%)" measureLocation={ProgressMeasureLocation.none} />;

@karelhala I think he is more speaking to line 56 here since in addition to the label passed the component is also adding in <span className={css(styles.progressMeasure)}>{value}%</span> that would make this:

<Progress value={33} title="Descriptive text here" label="$106.30 (82.49%)" />;

Output something like $106.30 (82.49%) 33% from the looks of it.

Is there a way that we can have them just override the actual measure label? like David mentioned, default to the {value}%, but user the one provided if there is one. Someone might want to override the label no matter the possition.

Mmmh, that makes sence, I was kinda confused as to what custom label meant (since one was already there).

patternfly-build · 2018-10-21T10:16:25Z

PatternFly-React preview: https://822-pr-patternfly-react-patternfly.surge.sh

dlabrecq · 2018-10-22T15:48:13Z

packages/patternfly-4/react-core/src/components/Progress/ProgressContainer.js

-          <span className={css(styles.progressMeasure)}>{value}%</span>
+          <span className={css(progressMeasure, measureLocation === ProgressMeasureLocation.top && progressLabel)}>
+            {value}%
+          </span>


I still feel that the label should override the value / measureLocation. Having to also set ProgressMeasureLocation.none doesn't feel clean, but don't feel strongly about it.

patternfly-build · 2018-10-23T15:37:02Z

Deploy preview for patternfly-react-gone ready!

Built with commit 531592b

https://deploy-preview-822--patternfly-react-gone.netlify.com

dlabrecq

This looks great. Now I can add my own label and still use measureLocation to place it in a desired location. Much better! Thanks

jschuler · 2018-10-24T15:58:19Z

packages/patternfly-4/react-core/src/components/Progress/Progress.js

@@ -21,6 +21,8 @@ const propTypes = {
  variant: PropTypes.oneOf(Object.values(ProgressVariant)),
  /** Title above progress. */
  title: PropTypes.string,
+  /** Label to indicate what progress is shwoing. */
+  label: PropTypes.oneOfType([PropTypes.string, PropTypes.node, PropTypes.number]),


node includes string and number

Suggested change

label: PropTypes.oneOfType([PropTypes.string, PropTypes.node, PropTypes.number]),

label: PropTypes.node,

jschuler · 2018-10-24T15:59:49Z

packages/patternfly-4/react-core/src/components/Progress/Progress.js

@@ -21,6 +21,8 @@ const propTypes = {
  variant: PropTypes.oneOf(Object.values(ProgressVariant)),
  /** Title above progress. */
  title: PropTypes.string,
+  /** Label to indicate what progress is shwoing. */


nit: spelling of 'showing'

jschuler · 2018-10-24T16:00:13Z

packages/patternfly-4/react-core/src/components/Progress/ProgressContainer.js

@@ -23,6 +28,8 @@ const propTypes = {
  parentId: PropTypes.string.isRequired,
  /** Progress title. */
  title: PropTypes.string,
+  /** Label to indicate what progress is shwoing. */
+  label: PropTypes.oneOfType([PropTypes.string, PropTypes.node, PropTypes.number]),


same comments as above

Suggested change

label: PropTypes.oneOfType([PropTypes.string, PropTypes.node, PropTypes.number]),

label: PropTypes.node,

…progressbar

karelhala requested review from dlabrecq and amarie401 October 20, 2018 22:09

dlabrecq requested changes Oct 21, 2018

View reviewed changes

karelhala force-pushed the progressbar-label branch from d0a886e to 353ea44 Compare October 21, 2018 10:03

karelhala added PF4 labels Oct 21, 2018

amarie401 requested review from jschuler and tlabaj October 21, 2018 17:11

karelhala force-pushed the progressbar-label branch from 353ea44 to f50624a Compare October 22, 2018 15:03

dlabrecq previously approved these changes Oct 22, 2018

View reviewed changes

karelhala dismissed dlabrecq’s stale review via 531592b October 23, 2018 15:27

karelhala force-pushed the progressbar-label branch from f50624a to 531592b Compare October 23, 2018 15:27

dlabrecq previously approved these changes Oct 23, 2018

View reviewed changes

jschuler reviewed Oct 24, 2018

View reviewed changes

feat(Labeled progress): allow consumers to pass additional label for …

34c11f1

…progressbar

karelhala dismissed dlabrecq’s stale review via 34c11f1 October 24, 2018 16:37

karelhala force-pushed the progressbar-label branch from 531592b to 34c11f1 Compare October 24, 2018 16:37

amarie401 approved these changes Oct 24, 2018

View reviewed changes

jschuler approved these changes Oct 24, 2018

View reviewed changes

jschuler merged commit b8ce0c9 into patternfly:master Oct 24, 2018

tlabaj mentioned this pull request Jan 15, 2019

PF4 Progress Bar: Need a unique property for percentage label #784

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(Labeled progress): allow consumers to pass additional label for progressbar #822

feat(Labeled progress): allow consumers to pass additional label for progressbar #822

karelhala commented Oct 20, 2018

coveralls commented Oct 20, 2018 •

edited

Loading

dlabrecq Oct 21, 2018 •

edited

Loading

karelhala Oct 21, 2018

dlabrecq Oct 22, 2018

karelhala Oct 22, 2018

dmiller9911 Oct 22, 2018

karelhala Oct 22, 2018

patternfly-build commented Oct 21, 2018

dlabrecq Oct 22, 2018 •

edited

Loading

patternfly-build commented Oct 23, 2018

dlabrecq left a comment

jschuler Oct 24, 2018

jschuler Oct 24, 2018

jschuler Oct 24, 2018

	label: PropTypes.oneOfType([PropTypes.string, PropTypes.node, PropTypes.number]),
	label: PropTypes.node,

feat(Labeled progress): allow consumers to pass additional label for progressbar #822

feat(Labeled progress): allow consumers to pass additional label for progressbar #822

Conversation

karelhala commented Oct 20, 2018

coveralls commented Oct 20, 2018 • edited Loading

Pull Request Test Coverage Report for Build 2796

💛 - Coveralls

dlabrecq Oct 21, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

patternfly-build commented Oct 21, 2018

dlabrecq Oct 22, 2018 • edited Loading

Choose a reason for hiding this comment

patternfly-build commented Oct 23, 2018

dlabrecq left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Oct 20, 2018 •

edited

Loading

dlabrecq Oct 21, 2018 •

edited

Loading

dlabrecq Oct 22, 2018 •

edited

Loading