Skip to content
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.

Sign up for GitHub

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

Jump to bottom

feat(time-estimator): add component #240

Merged
merged 13 commits into from
Dec 10, 2019
Merged

feat(time-estimator): add component #240

merged 13 commits into from
Dec 10, 2019

Conversation

SimonFinney
Copy link
Contributor

Affected issues

Proposed changes

  • Add TimeEstimator component
  • Add helpers for Storybook Component Story Format (CSF)
  • Use story as the basis for snapshot test

@netlify
Copy link

netlify bot commented Nov 27, 2019 •
edited
Loading

Deploy preview for ibm-security ready!

Built with commit cabdd2c

https://deploy-preview-240--ibm-security.netlify.com

@SimonFinney SimonFinney marked this pull request as ready for review December 4, 2019 22:07
@SimonFinney SimonFinney requested a review from a team as a code owner December 4, 2019 22:07
@SimonFinney SimonFinney requested review from jendowns, a team and petervachon and removed request for a team December 4, 2019 22:07
petervachon
petervachon reviewed Dec 5, 2019
View reviewed changes
Copy link

@petervachon petervachon left a comment

Choose a reason for hiding this comment

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

Very cool! I'm wondering how we may be able to standardize/govern the use of 'min' vs. 'minutes' 'hr' vs. 'hours'? Is it something we need to create an option for or just write documentation that outlines best practices, thoughts?

@SimonFinney
Copy link
Contributor Author

@petervachon Good point! Right now, this is pretty dumb as it only accepts a string so technically anything could be passed in.

I could document recommended usage directly in Storybook if you had any thoughts around it?

jendowns
jendowns previously requested changes Dec 5, 2019
View reviewed changes
Copy link
Contributor

@jendowns jendowns left a comment

Choose a reason for hiding this comment

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

Had some formatting/structure questions and then the bigger one for me is -- what makes this is "time estimator"? Could it just be any text next to any icon?

src/components/TimeEstimator/__tests__/TimeEstimator.spec.js Outdated Show resolved Hide resolved
src/components/TimeEstimator/index.js Outdated Show resolved Hide resolved
src/components/TimeEstimator/index.js Outdated Show resolved Hide resolved
.storybook/index.js Outdated Show resolved Hide resolved
src/components/TimeEstimator/index.js Outdated Show resolved Hide resolved
src/components/TimeEstimator/index.js Outdated Show resolved Hide resolved
@petervachon
Copy link

Usage documentation in Storybook would be great! It looks liked we do not want to abbreviate unless the space is too tight, which in our use case it is not, so we would use minute

@jendowns jendowns dismissed their stale review December 9, 2019 15:25

Dismissing my stale review

jendowns
jendowns approved these changes Dec 9, 2019
View reviewed changes
Copy link
Contributor

@jendowns jendowns left a comment

Choose a reason for hiding this comment

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

Looks great @SimonFinney!

One thing to note though -- the component always appears at the bottom of the "Components" section of the storybook. It's not in alphabetical order in the sidebar. 🤔

@SimonFinney SimonFinney merged commit b4e0520 into dev Dec 10, 2019
@SimonFinney SimonFinney deleted the 145/time-estimator branch December 10, 2019 11:19
SimonFinney pushed a commit that referenced this pull request Dec 10, 2019
@semantic-release-bot
chore(release): 1.10.2-prerelease.4 [skip ci]
4ea4b08 
## [1.10.2-prerelease.4](https://github.com/carbon-design-system/ibm-security/compare/[email protected]@canary) (2019-12-10)

### Features

* **time-estimator:** add component ([#240](#240)) ([b4e0520](b4e0520))
@SimonFinney
Copy link
Contributor Author

🎉 This PR is included in version 1.10.2-prerelease.4 🎉

The release is available on:

Your semantic-release bot 📦🚀

SimonFinney pushed a commit that referenced this pull request Dec 10, 2019
@semantic-release-bot
chore(release): 1.11.0 [skip ci]
242b865 
# [1.11.0](v1.10.1...v1.11.0) (2019-12-10)

### Bug Fixes

* **data-table:** fix theme tokens, reduce overrides ([#252](#252)) ([ccec2d6](ccec2d6))
* **header:** add hyphenated word break for popover title ([#250](#250)) ([495f531](495f531))

### Features

* **summary-card:** add multiselect, extend `DataTable` batch actions ([#228](#228)) ([c215958](c215958))
* **time-estimator:** add component ([#240](#240)) ([b4e0520](b4e0520))
@SimonFinney
Copy link
Contributor Author

🎉 This PR is included in version 1.11.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@petervachon petervachon petervachon left review comments

@jendowns jendowns jendowns approved these changes

Assignees
No one assigned
Labels
released on @canary released
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Time estimator / formatter
3 participants
@SimonFinney @petervachon @jendowns