Unit | Component | sl tab panel: getInitialTabName() returns the correct tab name

softlayer / sl-ember-components

An Ember CLI Addon that provides a variety of UI components.

http://softlayer.github.io/sl-ember-components

MIT License

114 stars 27 forks source link

Unit | Component | sl tab panel: getInitialTabName() returns the correct tab name #1639

Closed erangeles closed 8 years ago

erangeles commented 8 years ago

juwara0 commented 8 years ago

Reviewed 2 of 2 files at r1. Review status: all files reviewed at latest revision, 1 unresolved discussion.

tests/integration/components/sl-tab-panel-test.js, line 42 [r1] (raw file): I think we should also add the check for the active class on the tab panel.

Comments from the review on Reviewable.io

juwara0 commented 8 years ago

If this is the last of the tests using the template at the top: https://github.com/softlayer/sl-ember-components/blob/eb33a8027093cc0230b91c89c5dfa3d2a993be98/tests/integration/components/sl-tab-panel-test.js#L5-L11 then we can remove that code as well.

Review status: all files reviewed at latest revision, 1 unresolved discussion.

Comments from the review on Reviewable.io

juwara0 commented 8 years ago

This is the correct link: https://github.com/softlayer/sl-ember-components/blob/eb33a8027093cc0230b91c89c5dfa3d2a993be98/tests/unit/components/sl-tab-panel-test.js#L9-L21 The one above was to the integration test.

Review status: all files reviewed at latest revision, 1 unresolved discussion.

Comments from the review on Reviewable.io

erangeles commented 8 years ago

tests/integration/components/sl-tab-panel-test.js, line 42 [r1] (raw file): done.

Comments from the review on Reviewable.io

erangeles commented 8 years ago

verifying the template use and if it can be removed.

_Comments from the review on Reviewable.io_

juwara0 commented 8 years ago

Review status: 1 of 2 files reviewed at latest revision, 2 unresolved discussions.

tests/integration/components/sl-tab-panel-test.js, line 44 [r2] (raw file): do you need to add the .hasClass()

Comments from the review on Reviewable.io

juwara0 commented 8 years ago

Reviewed 1 of 1 files at r2. Review status: all files reviewed at latest revision, 2 unresolved discussions.

Comments from the review on Reviewable.io

erangeles commented 8 years ago

the unit test for setupTabs() still uses the template. I think you grabbed that test issue if im not mistaken.

Comments from the review on Reviewable.io

erangeles commented 8 years ago

Review status: 1 of 2 files reviewed at latest revision, 2 unresolved discussions.

tests/integration/components/sl-tab-panel-test.js, line 44 [r2] (raw file): Done.

Comments from the review on Reviewable.io

juwara0 commented 8 years ago

Yes, I removed that test from the unit tests.

Review status: 1 of 2 files reviewed at latest revision, all discussions resolved.

Comments from the review on Reviewable.io

juwara0 commented 8 years ago

Reviewed 1 of 1 files at r3. Review status: all files reviewed at latest revision, all discussions resolved.

Comments from the review on Reviewable.io

juwara0 commented 8 years ago

Reviewed 1 of 1 files at r4. Review status: all files reviewed at latest revision, all discussions resolved.

Comments from the review on Reviewable.io

juwara0 commented 8 years ago

@notmessenger - This PR should be merged last (Of the sl-tab-panel test issues, there are four) since it removes the last piece of needed code (the template)

Review status: all files reviewed at latest revision, all discussions resolved, some commit checks pending.

Comments from the review on Reviewable.io

notmessenger commented 8 years ago

Reviewed 1 of 1 files at r3, 1 of 1 files at r4. Review status: all files reviewed at latest revision, 1 unresolved discussion, some commit checks failed.

tests/unit/components/sl-tab-panel-test.js, line 153 [r4] (raw file): Why is this test being removed? The corresponding functionality in the component still remains.

Comments from the review on Reviewable.io

erangeles commented 8 years ago

tests/unit/components/sl-tab-panel-test.js, line 153 [r4] (raw file): @juwara0 and I decided

the removed unit tests were testing the dom(which is the integration tests job) including this one as it utilizes the test-template.
this has been replaced with an integration test named "first tab/tab pane has class active" to test the dom properly.

Comments from the review on Reviewable.io

notmessenger commented 8 years ago

Test is failing with

/home/travis/build/softlayer/sl-ember-components/tests/unit/components/sl-tab-panel-test.js
error  "hbs" is defined but never used  no-unused-vars

Review status: 0 of 2 files reviewed at latest revision, 1 unresolved discussion, some commit checks failed.

_Comments from the review on Reviewable.io_

notmessenger commented 8 years ago

Reviewed 2 of 2 files at r5. Review status: all files reviewed at latest revision, all discussions resolved, some commit checks failed.

Comments from the review on Reviewable.io

erangeles commented 8 years ago

ready to merge.

_Comments from the review on Reviewable.io_

notmessenger commented 8 years ago

Reviewed 1 of 1 files at r6. Review status: all files reviewed at latest revision, all discussions resolved, some commit checks failed.

Comments from the review on Reviewable.io