Skip to content

Add Progress Bar Component #1773

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

Merged
merged 18 commits into from
Apr 26, 2019
Merged

Conversation

aswinshenoy
Copy link
Contributor

@aswinshenoy aswinshenoy commented Feb 28, 2019

Fixes #1365

Added Progress Bar Component support.

@interactivellama @futuremint, kindly review it, I will make any changes required.

Additional description

The progress bar component as found in SLDS has been ported to react, with almost all its functionality, and reusing the same classes.

Props Taken

  • id - HTML id for the component container
  • className - CSS classes to be added to tag with .slds-progress-bar.
  • label - Label for the progress bar, if provided will render as a descriptive progress bar
  • color - Color to be filled in the progress bar
  • thickness - Thickness of the progress bar
  • value - Percentage of progress completion, ranging [0, 100].

Limitations

  • Vertical Progress bar has not been implemented as of now, but can easily be added
  • Color, Radius are limited to already defined classes in SLDS.

Screenshots

Default -
image
With Label -
image
Color set to success -
image
Different Thickness -
image
Rounded -
image


CONTRIBUTOR checklist (do not remove)

Please complete for every pull request

  • First-time contributors should sign the Contributor License Agreement. It's a fancy way of saying that you are giving away your contribution to this project. If you haven't before, wait a few minutes and a bot will comment on this pull request with instructions.
  • npm run lint:fix has been run and linting passes.
  • Review the appropriate Storybook stories. Open http://localhost:9001/.
  • The Accessibility panel of each Storybook story has 0 violations (aXe). Open http://localhost:9001/.
  • Mocha, Jest (Storyshots), and components/component-docs.json CI tests pass (npm test).
  • Tests have been added for new props to prevent regressions in the future. See readme.
  • Review tests are passing in the browser. Open http://localhost:8001/.
  • Review markup conforms to SLDS by looking at DOM snapshot strings.

REVIEWER checklist (do not remove)

  • TravisCI tests pass. This includes linting, Mocha, Jest, Storyshots, and components/component-docs.json tests.
  • Tests have been added for new props to prevent regressions in the future. See readme.
  • Review the appropriate Storybook stories. Open http://localhost:9001/.
  • The Accessibility panel of each Storybook story has 0 violations (aXe). Open http://localhost:9001/.
  • Review tests are passing in the browser. Open http://localhost:8001/.
  • Review markup conforms to SLDS by looking at DOM snapshot strings.
Required only if there are markup / UX changes
  • Add year-first date and commit SHA to last-slds-markup-review in package.json and push.
  • Request a review of the deployed Heroku app by the Salesforce UX Accessibility Team.
  • Add year-first review date, and commit SHA, last-accessibility-review, to package.json and push.
  • While the contributor's branch is checked out, run npm run local-update within locally cloned site repo to confirm the site will function correctly at the next release.

@aswinshenoy
Copy link
Contributor Author

I learning and working on adding test cases, as per given instructions in the contribution guide. I will update with a patch, adding it in sometime soon. :)

@interactivellama
Copy link
Contributor

@aswinshenoy Screenshots are looking great! I'll try to get you feedback on your props/API proposal soon.

@aswinshenoy
Copy link
Contributor Author

@interactivellama meanwhile, I will add the component into components/component-docs.json, and also try writing tests.
Are multiple commits per PR okay?

Regards,

@futuremint futuremint self-requested a review March 1, 2019 16:46
@futuremint futuremint self-assigned this Mar 1, 2019
@futuremint futuremint removed their request for review March 1, 2019 16:46
@futuremint futuremint removed their assignment Mar 1, 2019
@interactivellama
Copy link
Contributor

yes, multiple commits are fine.

Copy link
Contributor

@tanhengyeow tanhengyeow left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just some friendly pointers from me after taking a quick look at this PR :)

Can take them into consideration but I think @interactivellama would advice better 😄

@interactivellama
Copy link
Contributor

Props proposal is approved. Please ping when you are ready for a review.

@interactivellama
Copy link
Contributor

Props approved. Please proceed with tests, since this isn't interactive DOM and image snapshots should be enough.

@aswinshenoy The prop type for label should be able to be node and string.

@interactivellama
Copy link
Contributor

Also, you have lint errors

@aswinshenoy
Copy link
Contributor Author

I have added snapshot test, and written progress-bar.snapshot-test.jsx similar to that of progress-ring.snapshot-test.jsx.

@interactivellama interactivellama self-requested a review April 16, 2019 22:26
@interactivellama
Copy link
Contributor

@aswinshenoy Because I was unclear about how the Jest snapshot tests were to be created, I went ahead and pushed an update to your branch. 9dcb6d5#diff-ef6040eafe786a4a9723ca820a5e2566R43

The id prop in the examples need to be set so that the DOM snapshots are always the same when rendered.

@aswinshenoy
Copy link
Contributor Author

@interactivellama I have pushed adding ids in the examples, and the tests have passed it seems :) Sorry, I was a bit late and inactive for some days.

@interactivellama
Copy link
Contributor

@aswinshenoy I will be the last person to complain about being late and inactive (at least before and after the official GSoC time 😄 ).

@interactivellama
Copy link
Contributor

As an aside, @aswinshenoy it's great that you have added some of the building block components that will be consumed by Setup Assistant.

Copy link
Contributor

@interactivellama interactivellama left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I found a few bugs. Mostly the width issue.

this.props.className
)}
style={{
width: `${this.props.value}%`,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There isn't a gray background on the progress bar as in https://www.lightningdesignsystem.com/components/progress-bar/. This is because the "container" width is being set instead of slds-progress-bar__value.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I must have been a little careless there. :( Looks perfect with your patch 👍

>
<span>{this.props.label}</span>
<span aria-hidden="true">
<strong>{this.props.value}% Complete</strong>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All text should be able to be changed with props in order to support internationalization.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

True, I forgot about the internationalization part. Seems like you already fixed it with labels prop. Thank you :)

…eact into pr/aswinshenoy/1773

# Conflicts:
#	components/progress-bar/__examples__/default.jsx
#	components/progress-bar/__examples__/descriptive.jsx
@interactivellama interactivellama merged commit d289279 into salesforce:master Apr 26, 2019
@interactivellama
Copy link
Contributor

Looks good! Thank you @aswinshenoy

@aswinshenoy
Copy link
Contributor Author

@interactivellama It is wonderful to see it done :) I will looking forward for more 👍 Thank you so much for writing out the test, and doing all those changes. 💯 I had been a little sloppy on this, perhaps, sorry.

@interactivellama
Copy link
Contributor

I just talked to a program manager today on an internal team, and they are looking to consume this and ScopedNotification, so I wanted to go ahead and get this these merged.

@aswinshenoy
Copy link
Contributor Author

aswinshenoy commented Apr 26, 2019

@interactivellama that's cool, thank you. The #1785 PR that adds ScopedNotifications also has also passed the tests and seems it is mergeable now after your fixes.

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

Successfully merging this pull request may close these issues.

Add Progress Bar component
4 participants