-
Notifications
You must be signed in to change notification settings - Fork 0
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.
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
Created Students Show View #64
Conversation
Codecov Report
@@ Coverage Diff @@
## development #64 +/- ##
===============================================
+ Coverage 11.88% 12.04% +0.16%
===============================================
Files 96 97 +1
Lines 2626 2631 +5
Branches 175 175
===============================================
+ Hits 312 317 +5
Misses 2314 2314 Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Responsive behavior is not working ok. I'll fix this and all index.html
related issues for you. You take care of the rest.
'use strict'; | ||
|
||
define(['paw'], function(paw) { | ||
paw.controller('StudentsShowCtrl', ['$routeParams', function($routeParams) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fix indentation
'use strict'; | ||
|
||
define(['paw'], function(paw) { | ||
paw.controller('StudentsShowCtrl', ['$routeParams', function($routeParams) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice use of the array and string service injection! 😄
paw.controller('StudentsShowCtrl', ['$routeParams', function($routeParams) { | ||
var _this = this; | ||
|
||
this.docket = $routeParams.docket; // For future Service calls |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perfect! 👍
However, I wouldn't assign this to the controller, but to a variable, as it is not necessary to expose this information to the view.
i.e.:
var docket = $routeParams.docket; // For future Service calls
$controller = _$controller_; | ||
controller = $controller('StudentsShowCtrl'); | ||
expectedStudent = | ||
{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unify style: either you put this {
beside the above =
or you fix the address: {
line
|
||
it('loads the required student', function() { | ||
expect(controller.student).toEqual(expectedStudent); | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Test case missing: you should be testing that the controller's docket is being loaded from the $routeParams service. Take a look at the StudentsIndexCtrl.spec.js
from #63
However, if you've changed the docket assignment from this.docket
to var docket
as suggested above, you won't be able to test the controllers variable (as it is local to that function). What you would have to do is to use a Jasmine Spy and check that the $routeParam.docket is being requested. You can also check other spec files for further help. If you cannot manage it, just tell me.
<!-- Page Heading --> | ||
<div class="row"> | ||
<div class="col-xs-12"> | ||
<h2 class="page-header"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know that I've told you to use h2
as a page header in #63, but I made a mistake. Sorry for that. Please update this to h1
@@ -0,0 +1,125 @@ | |||
<div ng-controller='StudentsShowCtrl as controller'> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use simple quotes whenever possible. Ask me for private how to do it quickly.
<div class="row"> | ||
<div class="col-xs-12"> | ||
<h2 class="page-header"> | ||
<span translate="i18nStudentsPanelSection"></span> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Try to create another translation with the same text. I know, maybe it sounds kind of weird, but let me explain you.
Your string representing your translation should represent where you will use it, no matter what its text is. So, in this case, I would define a translation
i18nStudents: Alumnos
.
Trade off of this option: cleaner and semantic code, and independent key-value translation per view vs. wording repetition.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not even sure why it's there. It should the the student's firstName + lastName.
@@ -11,6 +11,10 @@ define([], function() { | |||
'/login': { | |||
templateUrl: '/views/login.html', | |||
controller: 'LoginCtrl' | |||
}, | |||
'/student/:docket': { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This path is not OK. it should be /students/:docket
('s' from students missing)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My fixes are pending too. I'll ping you when I've finished
@@ -12,36 +12,39 @@ function() { | |||
describe('Students Show Ctrl', function() { | |||
beforeEach(module('paw')); | |||
|
|||
var $controller, controller, expectedStudent; | |||
var expectedStudent = expectedStudent = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's this? I haven't seen it before...
I'll change it, but if you intentionally wrote this, please explain it to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, definitely not intentional.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No problem bro! 😄
c519f89
to
1bf71da
Compare
1bf71da
to
fcfd4f7
Compare
Students Show View
The view shows a student's detail. It can be access at /students/:docket.
Since student is hard coded, the view can be accessed using any docket.
Trello Card
https://trello.com/c/c16h7OXj/34-maquetar-vista-de-un-alumno