-
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 Courses Show View #73
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 one! Just a few fixes
var _this = this; | ||
var courseId = $routeParams.courseId; // For future Service calls | ||
|
||
this.formatDate = function(finalExamDate) { |
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 should not be here. Controllers don't have to know how data should be manipulated or styled. They should called Angular's filters to perform these kind of tasks.
Move this logic to a filter.
'semester': 3 | ||
}; | ||
|
||
this.correlatives = [ |
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.
Correlatives information should be related to the course, because they are correlatives of that course. Nest this correlatives inside the above course definition. That's the way data should be received by the controller when fetching it from the Courses service.
} | ||
]; | ||
|
||
this.finalInscriptions = [ |
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.
var expectedCourseId = '93.42'; | ||
var expectedParams = {courseId: expectedCourseId}; | ||
|
||
var expectedCourse = { |
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.
Update all this object to match the one modified with the suggestions. Also, use the apiResonses API to store data there and retrieve it on different tests.
There are few examples of its usage all along other tests.
it('correctly fetch the final inscriptions', function() { | ||
expect(controller.finalInscriptions).toEqual(expectedFinalInscriptions); | ||
}); | ||
it('correctly format the final exam date', function() { |
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 is the filter test case 😄
@@ -0,0 +1,35 @@ | |||
@import 'modules/variables/importer'; |
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 this style out later, but syntax and structure seem OK
app/views/courses/show.html
Outdated
<td colspan='4' class='text-danger text-center' translate='i18nNoFinalExamsFound'></td> | ||
</tr> | ||
<tr ng-repeat='inscription in controller.finalInscriptions'> | ||
<td>{{controller.formatDate(inscription.finalExamDate)}}</td> |
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.
Filter here (just to remind you where you were using the method)
app/views/courses/show.html
Outdated
@@ -0,0 +1,107 @@ | |||
<div class='courses-show-container' ng-controller='CoursesShowCtrl 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.
I'll check this view out later, but syntax and structure seem OK
@@ -3,10 +3,10 @@ <h4 class='modal-title' id='modal-title' translate='i18nCourseDelete'></h4> | |||
</div> | |||
<div class='modal-body'> | |||
<p class='break-words'> | |||
<span class='bold' translate='i18nId'></span>: {{ controller.id }} | |||
<span class='bold' translate='i18nCourseId'></span>: {{ controller.id }} |
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.
Good catches! 😄
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.
The rest of the style is pretty nice! Congrats! 🎉
display: flex; | ||
flex-wrap: wrap; | ||
width: 100%; | ||
justify-content: space-between; |
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 this: justify-content: flex-start;
.btn { | ||
flex: 1; | ||
min-width: 175px; | ||
max-width: 250px; |
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 this:
max-width: 200px;
margin: 10px;
margin-left: 0;
justify-content: space-between; | ||
|
||
.data-item { | ||
width: 230px; |
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 this: width: 200px;
.break-words { | ||
display: inline-block; | ||
word-wrap: break-word; /* Internet Explorer 5.5+ */ | ||
width: 220px; /* width is mandatory to make break-word work on Mozzila */ |
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 this: 190px
app/views/courses/show.html
Outdated
<!-- /Course Data --> | ||
|
||
<!-- Correlatives --> | ||
<h3 translate='i18nCorrelatives' class='page-header'></h3> |
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 would apply page-header
class to this item, but yes a similar style.
I would create a class page-sub-header
with the same style as the page header, and additionally, a padding-top: 15px
so as to separate the previous content of the current one.
app/views/courses/show.html
Outdated
<!-- /Correlatives --> | ||
|
||
<!-- Final Exams --> | ||
<h3 translate='i18nFinalExams' class='page-header'></h3> |
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 'Correlatives' subheader
Add style for actions buttons
585f31a
to
58b6c16
Compare
* Created Courses Show View Add style for actions buttons Add style for h3 headers
Courses Show View
The view contains the course's information, correlatives and final inscriptions.
Trello Card
https://trello.com/c/pqYuXGRU/49-maquetar-vista-de-curso