-
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 Update View #67
Conversation
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 work! Only those changes and then I'll give it a try to the view before merging it! 😄
i18nDni: 'DNI', | ||
i18Docket: 'Legajo', | ||
i18nDocket: 'Legajo', |
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 for not having seen this before :(
@@ -16,6 +16,10 @@ define([], function() { | |||
templateUrl: 'views/students/index.html', | |||
controller: 'StudentsIndexCtrl', | |||
relativePath: '/students' | |||
}, | |||
'/students/:docket/update': { |
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.
It should be /students/:docket/edit
.
update
is the POST action that will be send to the server.
Why have you changed this from the doc? Please fix it also there
Also, rename the controller to be StudentsEditCtrl
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 first marked it as conflict, since both StudentsUpdate and StudentsEdit were added to the documentation, but only StudentsUpdate exists in API.
I now deleted the resources marked as conflict, to clear the documentation.
Finally, I changed the view's path to /update to maintain consistency. This last change is of course, not required since API and Front Paths don't need to match.
Why would you choose /edit over /update?
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.
It's just that we've agreed to use Rail's endpoints as a starting point.
I've told you to change it since there the edit's view path is /photos/:id/edit
.
It's just fu**ing convention. We should play 'stone, paper or scissors' to define it 😝
What do you say?
Also, I have to admit that I've made a mistake. Note that the update path is /photos/:id
with a PATCH/PUT HTTP verb (there is no /update
at the end of the URL).
I've noticed that in the documentation we have used a POST verb. @gibarsin would you change it or leave it like that?
@@ -16,6 +16,10 @@ define([], function() { | |||
templateUrl: 'views/students/index.html', | |||
controller: 'StudentsIndexCtrl', | |||
relativePath: '/students' | |||
}, | |||
'/students/:docket/update': { | |||
templateUrl: '/views/students/update.html', |
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.
Note that views are now written without the starting slash. Fix this or it won't work on production environment.
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.
Done!
}, | ||
'/students/:docket/update': { | ||
templateUrl: '/views/students/update.html', | ||
controller: 'StudentsUpdateCtrl' |
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 controller should have set the relativePath: '/students'
property, as the other controllers from above. When doing this, you should move this controller to the 'controllers/students' folder, as the others.
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.
Will keep it in mind!
@@ -83,3 +83,9 @@ hr { | |||
.btn { | |||
border-radius: 0; | |||
} | |||
|
|||
.full-width-button { |
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'll check it later directly on the view
@@ -0,0 +1,11 @@ | |||
.students-update-container { |
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.
Same as above
</tr> | ||
<tr ng-repeat='student in controller.students'> | ||
<td>{{ student.docket }}</td> | ||
<td>{{ student.firstName }}</td> | ||
<td>{{ student.lastName }}</td> | ||
<td class='actions-container'> | ||
<a class='btn btn-default fullWidthButton' role='button'> | ||
<a class='btn btn-default full-width-button' role='button'> |
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.
Now, reading this, I think that this class (full-width-button
) is not necessary, as we are using flex to fill the space depending on the amount of action buttons. I'll check it later.
But good catch with the class name style!
@@ -0,0 +1,145 @@ | |||
<div ng-controller='StudentsUpdateCtrl as controller' class='students-update-container'> |
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.
Once I've finished with the view of #64, we can check it is possible to use that page header style instead of this one. The rest of this style I think it would be OK.
ff709cb
to
f57d759
Compare
Checking out branch Finished Students Update View Add test & rebased Development Fix bug in test Fix view & controller path in route.js Renamed StudentsUpdateCtrl to StudentsEditCtrl
f57d759
to
4a9b20a
Compare
- Fix edit page header - Update styles - Improve responsive style - Clone show style - Align texts - Style buttons - Fix some styles and translations on views - Add angular-messages dependency for form errors - Add form errors view and style - Add form error messages to all required fields - Fix index filters issue - Add toy update and cancel form functions - Add form validations trigger on submission - Add redirect after successful update - Add return to previous window location when cancel - Add new test cases
4a9b20a
to
2cdb557
Compare
* Add students edit view - Finished Students Update View - Add test - Renamed StudentsUpdateCtrl to StudentsEditCtrl - Update styles - Improve responsive style - Clone show style - Align texts - Style buttons - Add angular-messages dependency for form errors - Add form errors view and style - Add form error messages to all required fields - Add toy update and cancel form functions - Add form validations trigger on submission - Add redirect after successful update - Add return to previous window location when cancel - Add new test cases
Students Update View
Created the view for updating students.
Changed the view's path from /students/:docket/edit (as it is in the Master version) to /students/:docket/update
Trello Card
https://trello.com/c/upmdEyGZ/36-maquetar-vista-de-edicion-de-alumno