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

Request integration of Modules Taken Before Internship feature #34

Merged
merged 34 commits into from
Mar 17, 2017

Conversation

tgqiang
Copy link
Contributor

@tgqiang tgqiang commented Mar 15, 2017

Changes implemented in this branch:

  1. Back-end for this feature implemented by @nlzz22
  2. Front-end for this feature is implemented
  3. List of UI changes:
    a) Added hyperlinking to module codes for pages for Non-Overlapping Modules, Modules Taken Prior To Others
    b) Renaming, reordering for home page and sidebar links
    c) Standardizing navigation options section for Module Information, Fixed and Tentative Module Mounting pages

tgqiang and others added 29 commits March 14, 2017 14:00
@tgqiang tgqiang requested a review from helloqx March 15, 2017 10:49
@@ -40,7 +40,8 @@
'components.handlers.students_affected_by_module.StudentsAffectedByModule',
'/addModule', 'components.handlers.add_module_handler.AddModule',
'/moduleTakenPriorToOthers', 'components.handlers.module_taken_prior_to_others.TakePriorTo',
'/nonOverlappingModules', 'components.handlers.non_overlapping_modules.NonOverlappingModules'
'/nonOverlappingModules', 'components.handlers.non_overlapping_modules.NonOverlappingModules',
'/moduleTakenPriorToInternship', 'components.handlers.modules_taken_prior_to_internship.TakePriorInternship'
Copy link
Contributor

Choose a reason for hiding this comment

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

pylint: Line too long

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think this pylint issue should be ignored to make reading and modifying of the routes more easily.

# will have input data as function is called from button
input_data = web.input()
ay_sem = input_data.sem
raise web.seeother('/moduleTakenPriorToInternship?sem=' + ay_sem)
Copy link
Contributor

Choose a reason for hiding this comment

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

pylint: Missing final newline

test/test_app.py Outdated
response = root.click(linkid="home-page", href="/nonOverlappingModules")

Copy link
Contributor

Choose a reason for hiding this comment

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

pylint: Trailing whitespace

Copy link
Contributor

@helloqx helloqx left a comment

Choose a reason for hiding this comment

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

a) Added hyperlinking to module codes for pages for Non-Overlapping Modules, Modules Taken Prior To Others ✅

b) Renaming, reordering for home page and sidebar links ✅
See other comments

c) Standardizing navigation options section for Module Information, Fixed and Tentative Module Mounting pages ✅
Not urgent, but this box is pretty too huge

</div>
<br>

<table class="table display dataTable table-bordered table-hover text-center" id="modules-taken-prior-intern-table">
Copy link
Contributor

Choose a reason for hiding this comment

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

Should table be sortable?

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, when no data, vs when data exists, column sizes vary greatly:
image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should table be sortable?

Well it depends on what the users are looking for.
If they only want a list of modules, ignoring the number of students, sorting by module code/name will make it easier for them to check with their own list of modules.

If they want to consider the number of students as the primary concern, then sorting by number of students should be allowed.

Also, when no data, vs when data exists, column sizes vary greatly:

If really needed, I can try to find a fix for this (although this will NOT be in the priority list for UI fixes).

Copy link
Contributor

@helloqx helloqx Mar 17, 2017

Choose a reason for hiding this comment

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

Should table be sortable?

Yeah, right now the table does not have sort/search, unlike the other pages

Copy link
Contributor Author

Choose a reason for hiding this comment

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

image

Really? The sortings exist on my side.

Copy link
Contributor

Choose a reason for hiding this comment

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

😱 appeared after resetting again
Resolved

<li><a href="#">Before Specified Module</a></li>
<li><a href="#">Before Internship</a></li>
</ul-->
<li><a href="javascript:void(0)">Prior Modules Taken</a></li>
Copy link
Contributor

Choose a reason for hiding this comment

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

Useless link that closes sidebar, consider disabling

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sidebar will be redesigned at one of the refinement sprints. Please raise an issue in Github for this UI issue.

Copy link
Contributor

Choose a reason for hiding this comment

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

#38

Copy link
Contributor

@a0129998 a0129998 left a comment

Choose a reason for hiding this comment

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

Minor changes requested

  1. bold all modules in description of module information to match the style of fixed module mountings and tentative module mountings
  2. swap button value and tooltip of the buttons tentative module mountings and fixed module mountings

Copy link
Contributor

@a0129998 a0129998 left a comment

Choose a reason for hiding this comment

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

Requested changes fulfilled, all functionalities implemented work as expected,

@a0129998 a0129998 merged commit 591f0d4 into integration Mar 17, 2017
@a0129998 a0129998 deleted the modsBeforeInternship branch March 17, 2017 14:50
@a0129998 a0129998 restored the modsBeforeInternship branch March 17, 2017 14:51
@helloqx helloqx deleted the modsBeforeInternship branch March 17, 2017 14:55
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.

4 participants