Skip to content

Conversation

bryk
Copy link
Contributor

@bryk bryk commented Jun 3, 2016

To test this edit a pod in a list.

Missing things that will be done in next PR:

  • make the menu and popup look pretty
  • make json editor work on serve:prod (now images are missing)

This change is Reviewable

@bryk
Copy link
Contributor Author

bryk commented Jun 3, 2016

@floreks Can you review?

@edouardKaiser
Copy link
Contributor

Is it gonna land in the next beta ? Would LOVE to test that.

@codecov-io
Copy link

Current coverage is 94.96%

Merging #807 into master will increase coverage by 0.53%

  1. File ...ce/verber_service.js was modified. more
    • Misses -1
    • Partials 0
    • Hits +1
  2. File ...ereader_directive.js (not in diff) was modified. more
    • Misses -6
    • Partials 0
    • Hits +6
@@             master       #807   diff @@
==========================================
  Files           202        205     +3   
  Lines          1630       1667    +37   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits           1539       1583    +44   
+ Misses           91         84     -7   
  Partials          0          0          

Powered by Codecov. Last updated by 7a68617...c0982ab

@bryk
Copy link
Contributor Author

bryk commented Jun 3, 2016

Is it gonna land in the next beta ? Would LOVE to test that.

Awesome. Expect it later today or on Monday :) I'll let you know.

@floreks
Copy link
Member

floreks commented Jun 3, 2016

Awesome feature. Just few comments about i18n. Overall :lgtm:

I've noticed that travis job for this PR takes about 2x longer than other jobs. We should investigate that before merging. It looks like it hangs for some time on backend tests.

Previously, bryk (Piotr Bryk) wrote…

Is it gonna land in the next beta ? Would LOVE to test that.

Awesome. Expect it later today or on Monday :) I'll let you know.


Reviewed 14 of 18 files at r1, 1 of 1 files at r2.
Review status: 14 of 18 files reviewed at latest revision, 3 unresolved discussions.


src/app/frontend/common/components/resourcecard/resourcecardeditmenuitem.html, line 19 [r2] (raw file):

<md-menu-item>
  <md-button ng-click="$ctrl.edit()">
    Edit

i18n please :)


src/app/frontend/common/resource/deleteresource.html, line 19 [r2] (raw file):

<md-dialog aria-label="Delete Replication Controller" layout="column">
  <md-dialog-content layout-padding>
    <h4 class="md-title">Delete a {{$ctrl.resourceKindName}}</h4>

i18n?


src/app/frontend/common/resource/editresource.html, line 25 [r2] (raw file):

    </div>
    <md-dialog-actions>
      <md-button class="md-primary kd-cancel-btn" ng-click="$ctrl.cancel()">Cancel</md-button>

i18n?


Comments from Reviewable

@bryk
Copy link
Contributor Author

bryk commented Jun 3, 2016

I've noticed that travis job for this PR takes about 2x longer than other jobs. We should investigate that before merging. It looks like it hangs for some time on backend tests.

It is frontend-copies task that takes long. I'll investigate. It is slow because we include new bower dep.


Review status: 14 of 18 files reviewed at latest revision, 3 unresolved discussions.


Comments from Reviewable

To test this edit a pod in a list.

Missing things that will be done in next PR:
* make the menu and popup look pretty
* make json editor work on serve:prod (now images are missing)
@bryk
Copy link
Contributor Author

bryk commented Jun 3, 2016

All done. Will merge soon.

Previously, bryk (Piotr Bryk) wrote…

I've noticed that travis job for this PR takes about 2x longer than other jobs. We should investigate that before merging. It looks like it hangs for some time on backend tests.

It is frontend-copies task that takes long. I'll investigate. It is slow because we include new bower dep.


Review status: 12 of 27 files reviewed at latest revision, 3 unresolved discussions.


src/app/frontend/common/components/resourcecard/resourcecardeditmenuitem.html, line 19 [r2] (raw file):

Previously, floreks (Sebastian Florek) wrote…

i18n please :)

Done.

src/app/frontend/common/resource/editresource.html, line 25 [r2] (raw file):

Previously, floreks (Sebastian Florek) wrote…

i18n?

Done.

Comments from Reviewable

@bryk
Copy link
Contributor Author

bryk commented Jun 3, 2016

Reviewed 12 of 18 files at r1, 15 of 15 files at r3.
Review status: all files reviewed at latest revision, all discussions resolved.


Comments from Reviewable

@bryk bryk merged commit 7ed5ea6 into kubernetes:master Jun 3, 2016
@bryk bryk deleted the yaml-json branch June 3, 2016 13:20
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