Skip to content

Conversation

pahosler
Copy link
Contributor

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Config Change ( change to config or build process )
  • I have read the CONTRIBUTING document.
  • My code follows the code style of this project.
  • My change requires a change to the documentation.
    • I have updated the documentation accordingly.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

@codecov-io
Copy link

codecov-io commented Oct 31, 2018

Codecov Report

Merging #47 into master will increase coverage by 0.02%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #47      +/-   ##
==========================================
+ Coverage    98.6%   98.63%   +0.02%     
==========================================
  Files          49       50       +1     
  Lines         430      439       +9     
==========================================
+ Hits          424      433       +9     
  Misses          6        6
Impacted Files Coverage Δ
src/Toast/Toast.js 100% <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 2f455b9...8e935d7. Read the comment docs.

@CodeDraken
Copy link
Owner

Needs some changes:

  1. You're not passing along attributes on the Toast component. This means users can't add any custom values to it. It also made the build fail - run npm run lint along with npm run lint-fix next time. ( these are in the readme )

/src/Toast/Toast.js:19:51: 'attributes' is assigned a value but never used.

  1. Check your test cases. This one says one thing and does another. Clean up the comments too.
it('Should render a span tag by default', () => {
    const wrapper = shallow(<Toast />)
     expect(wrapper.type()).toBe('div')
  })
  1. Unexpected changes - apparently you or something changed: the docs folder, package.json, pull request template and possibly other files. For example why did this get added in package.json?

+"@types/classnames": "^2.2.6",

Remove these unexpected changes.

package.json Outdated
@@ -63,7 +63,6 @@
"@storybook/node-logger": "^3.4.10",
"@storybook/react": "^3.4.10",
"@storybook/storybook-deployer": "^2.3.0",
"@types/classnames": "^2.2.6",
Copy link
Owner

Choose a reason for hiding this comment

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

Package.json should not be changed

@@ -1,4 +1,4 @@
# title-component
# add-Toast
Copy link
Owner

Choose a reason for hiding this comment

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

PULL_REQUEST_TEMPLATE.md should not be changed

@@ -17,7 +17,7 @@ describe('Toasts', () => {
})

it('Should pass additional classNames', () => {
const wrapper = shallow(<Toast className='extra'>My Toasts</Toast>)
const wrapper = shallow(<Toast className="extra">My Toasts</Toast>)
Copy link
Owner

Choose a reason for hiding this comment

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

Inconsistent quotes here. Use single quotes, follow the StandardJS style, run npm run lint to check for errors or use the extension.

const classNames = classnames(
{
[`toast-${color}`]: color,
toast: 'toast'
Copy link
Owner

Choose a reason for hiding this comment

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

Static classnames should not be inside the object for classnames().

expect(wrapper).toMatchSnapshot()
})

// it('Should have Toasts classname', () => {
Copy link
Owner

Choose a reason for hiding this comment

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

Cleanup comments

<Container>
<Row>
<Col all={3}>
<Toast className="m-2">
Copy link
Owner

Choose a reason for hiding this comment

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

Inconsistent quotes, needs to follow the StandardJS style guidelines using single quotes.

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

Successfully merging this pull request may close these issues.

3 participants