Skip to content

Conversation

floreks
Copy link
Member

@floreks floreks commented Jun 3, 2016

Closes #779

Simple pet set detail page with related pods.

zrzut ekranu z 2016-06-03 09-45-42


This change is Reviewable

@codecov-io
Copy link

Current coverage is 94.35%

Merging #805 into master will decrease coverage by 0.07%

  1. File ...ereader_directive.js (not in diff) was modified. more
    • Misses -6
    • Partials 0
    • Hits +6
@@             master       #805   diff @@
==========================================
  Files           197        202     +5   
  Lines          1596       1627    +31   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits           1508       1535    +27   
- Misses           88         92     +4   
  Partials          0          0          

Powered by Codecov. Last updated by bc3c542...49dfe23

@bryk
Copy link
Contributor

bryk commented Jun 3, 2016

Are we missing tests?

Previously, codecov-io wrote…

[Current coverage][cc-pull] is 94.35%

Merging [#805][cc-pull] into [master][cc-base-branch] will decrease coverage by 0.07%

  1. File ...ereader_directive.js (not in diff) was modified. more
    • Misses -6
    • Partials 0
    • Hits +6
@@             master       #805   diff @@
==========================================
  Files           197        202     +5   
  Lines          1596       1627    +31   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits           1508       1535    +27   
- Misses           88         92     +4   
  Partials          0          0          

Powered by Codecov. Last updated by [bc3c542...49dfe23][cc-compare]
[cc-base-branch]: https://codecov.io/gh/kubernetes/dashboard/branch/master?src=pr
[cc-compare]: https://codecov.io/gh/kubernetes/dashboard/compare/bc3c542f5e167bda8463a456e23dce8713ffb30e...49dfe23f65c7f80a69cc04870b3b5b95562f257b
[cc-pull]: https://codecov.io/gh/kubernetes/dashboard/pull/805?src=pr


Reviewed 16 of 16 files at r1.
Review status: all files reviewed at latest revision, 5 unresolved discussions.


src/app/frontend/petsetdetail/petsetdetail.html, line 20 [r1] (raw file):

  <md-content>
    <md-tabs md-border-bottom md-dynamic-height>
      <md-tab label="Overview">

I18n this?


src/app/frontend/petsetdetail/petsetdetail.html, line 24 [r1] (raw file):

        <kd-content-card>
          <kd-title>Pods</kd-title>

I18n this?


src/app/frontend/petsetdetail/petsetdetail_state.js, line 24 [r1] (raw file):

 * @final
 */
export class StateParams {

Don't use StateParams now. There's a common way of doing this. See how other detail pages do this now.


src/app/frontend/petsetdetail/petsetdetail_stateconfig.js, line 30 [r1] (raw file):

export default function stateConfig($stateProvider) {
  $stateProvider.state(stateName, {
    url: `${stateUrl}/:namespace/:petSet`,

There's a common way of crafting urls for detail pages. See other files.


src/app/frontend/petsetdetail/petsetinfo.html, line 18 [r1] (raw file):

<kd-info-card>
  <kd-info-card-header>Resource details</kd-info-card-header>

I18n this?


Comments from Reviewable

@floreks
Copy link
Member Author

floreks commented Jun 3, 2016

Yep. I've added them.

Previously, bryk (Piotr Bryk) wrote…

Are we missing tests?


Review status: 9 of 21 files reviewed at latest revision, 5 unresolved discussions.


src/app/frontend/petsetdetail/petsetdetail.html, line 20 [r1] (raw file):

Previously, bryk (Piotr Bryk) wrote…

I18n this?

Done.

src/app/frontend/petsetdetail/petsetdetail.html, line 24 [r1] (raw file):

Previously, bryk (Piotr Bryk) wrote…

I18n this?

Done.

src/app/frontend/petsetdetail/petsetdetail_state.js, line 24 [r1] (raw file):

Previously, bryk (Piotr Bryk) wrote…

Don't use StateParams now. There's a common way of doing this. See how other detail pages do this now.

Done.

src/app/frontend/petsetdetail/petsetdetail_stateconfig.js, line 30 [r1] (raw file):

Previously, bryk (Piotr Bryk) wrote…

There's a common way of crafting urls for detail pages. See other files.

Done.

src/app/frontend/petsetdetail/petsetinfo.html, line 18 [r1] (raw file):

Previously, bryk (Piotr Bryk) wrote…

I18n this?

Done.

Comments from Reviewable

@bryk
Copy link
Contributor

bryk commented Jun 3, 2016

:lgtm:

Previously, floreks (Sebastian Florek) wrote…

Yep. I've added them.


Reviewed 12 of 12 files at r2.
Review status: all files reviewed at latest revision, all discussions resolved.


Comments from Reviewable

@floreks floreks merged commit 7a68617 into kubernetes:master Jun 3, 2016
@floreks floreks deleted the petset-details branch June 3, 2016 09:02
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.

5 participants