Skip to content
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 Index View #63

Merged
merged 1 commit into from
Jan 29, 2017
Merged

Conversation

MatiasMercado
Copy link
Collaborator

Students Index View

A view containing the students (No API requests are being made)

Trello Card

https://trello.com/c/XfmDxa3Z/35-maquetar-vista-de-alumnos

@codecov-io
Copy link

codecov-io commented Jan 28, 2017

Current coverage is 11.88% (diff: 100%)

Merging #63 into development will increase coverage by 0.23%

@@           development        #63   diff @@
=============================================
  Files               95         96     +1   
  Lines             2619       2626     +7   
  Methods              0          0          
  Messages             0          0          
  Branches           192        192          
=============================================
+ Hits               305        312     +7   
  Misses            2314       2314          
  Partials             0          0          

Powered by Codecov. Last update ef09424...b1a8553

i18SearchBy: 'Buscar por',
i18SearchButton: 'Buscar',
i18ResetButton: 'Resetear',
i18NoStudentsFound: 'No se encontraron estudiantes'
Copy link
Owner

Choose a reason for hiding this comment

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

Use the web convention: 'No se encontraron alumnos'

@@ -11,6 +11,10 @@ define([], function() {
'/login': {
templateUrl: '/views/login.html',
controller: 'LoginCtrl'
},
'/students': {
templateUrl: '/views/students/_index.html',
Copy link
Owner

Choose a reason for hiding this comment

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

Update view's name to be /views/students/index.html.

apiResponsesService;

beforeEach(inject(
function(_$controller_, _$rootScope_, specUtils, apiResponses) {
Copy link
Owner

Choose a reason for hiding this comment

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

specUtils and apiResponses are not being used. Remove them from here and its respective assignments below.

it('clears the lastName search input', function() {
expect(controller.lastName).toEqual('');
});
});
Copy link
Owner

Choose a reason for hiding this comment

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

Test cases are missing. You are not testing that the controller is correctly loading the students array. This is not a problem up to now, but it may be when changing the hardcoded data with an API/Service call.
I would add this case just as to be sure we are adding core's controller data somewhere.

@@ -12,3 +12,4 @@ $fa-font-path: "/bower_components/font-awesome/fonts";
@import 'directives/backdrop';
@import 'partials/index';
@import 'partials/home';
@import 'partials/students_index';
Copy link
Owner

Choose a reason for hiding this comment

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

You should use the same view's folder convention. This is:

partials
|- students
   |- _index.scss

Please fix this.

<div ng-controller='StudentsIndexCtrl as controller'>
<div id="wrapper">
<div id="page-wrapper">
<div class="container-fluid">
Copy link
Owner

Choose a reason for hiding this comment

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

There is already a container class applied in the main's web index and bootstrap containers should not be nested. Please check this.

<!-- Page Heading -->
<div class="row">
<div class="col-xs-12">
<h1 translate="i18nStudentsPanelSection" class="page-header"></h1>
Copy link
Owner

Choose a reason for hiding this comment

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

I would use h2 here instead.

<div class="col-xs-12">
<div class="input-group">
<span class="input-group-addon search-label" translate=i18Docket></span>
<input type="text" class="form-control" ng-model="controller.docket" placeholder="{{ 'i18Docket' | translate }}..."/>
Copy link
Owner

Choose a reason for hiding this comment

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

Update this with the controller's update

<div class="col-xs-12">
<div class="input-group">
<span class="input-group-addon search-label" translate="i18nFirstName"></span>
<input type="text" class="form-control" ng-model="controller.firstName" placeholder="{{ 'i18nFirstName' | translate }}..."/>
Copy link
Owner

Choose a reason for hiding this comment

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

Same as above

<div class="col-xs-12">
<div class="input-group">
<span class="input-group-addon search-label" translate="i18nLastName"></span>
<input type="text" class="form-control" ng-model="controller.lastName" placeholder="{{ 'i18nLastName' | translate }}..."/>
Copy link
Owner

Choose a reason for hiding this comment

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

Same as above

@MatiasComercio
Copy link
Owner

Also, we should all follow this git style convention
It's nice explained! 😄

@MatiasComercio
Copy link
Owner

@MatiasMercado Wait for me to help you with this PR changes, and I'll ping you when you can fix the rest

@MatiasComercio
Copy link
Owner

@MatiasMercado I've forgot to tell you to add this PR link as a comment on the corresponding Trello card

Fix PR - 1

- Fix bootstrap style issues
- Refactor filter style
- Reorganize some styles files
- Update StudentsIndexCtrl to use $routeParams to grab filter's values

Removed students _index.scss
@MatiasMercado MatiasMercado merged commit a925c47 into development Jan 29, 2017
@MatiasMercado MatiasMercado deleted the students_index_view branch January 29, 2017 22:13
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