FinalsClub / karmaworld

KarmaNotes.org v3.0
GNU Affero General Public License v3.0
7 stars 6 forks source link

Note sorting on the Course pages needs work #266

Closed btbonval closed 10 years ago

btbonval commented 10 years ago

There are a few problems here.

  1. Notes used to be in order of upload (chronologically) with the newest Notes on top. This is no longer the case. Design question: do we want to order by timestamp or social media likes by default?
  2. The courses list page has a sort tool that allows the user to choose whether to sort the course list by date or the number of notes in the course. Design proposal: let's add this feature to the course page.
charlesconnell commented 10 years ago

I set it to -upload_date because it was driving me crazy, in a63ae38d3ee939a3c925050d2ecb9b2d7a6629ea. But I think a more complex ordering could be useful, like you suggested.

btbonval commented 10 years ago

This should be done by Tuesday. Promoting priority to urgent.

Ensure both popularity/likes and date are part of the sorting button thingy that will be added to the front/course page.

btbonval commented 10 years ago

There is so much custom stuff done to dataTables, both in the CSS and the JS. I'm wondering how much is really necessary as I copypasta all kinds of spaghetti from course_list to course_detail :|

btbonval commented 10 years ago

For example, this seems redundant. In the javascript, columns are identified as usable or not. https://github.com/FinalsClub/karmaworld/blob/675f7b061f2a96f3ce87ac92bda5c2f8554a0578/karmaworld/templates/courses/course_list.html#L44-L52 But then in the css, those columns are hidden anyway. https://github.com/FinalsClub/karmaworld/blob/c3b37c3a6491de4e4fe3e812e25e3bc53d1a697f/karmaworld/assets/css/course_list_table.css#L65-L75

CSS classes are used like IDs. I mean, it's make more sense to have

th.nodisplay {
  display: none;
}

and apply class="nodisplay" on the relevant th tags.

I don't understand why each th tag needs its own, uniquely identifying class. That's what IDs are for, but even still, I don't understand if this was a workaround to some other problem or what?

btbonval commented 10 years ago

Ran into a problem with oCol and found out why the above linked column stuff is required: http://stackoverflow.com/questions/6405429/ocol-is-undefined-using-datatables-and-jquery-ui-dialog#6409104

btbonval commented 10 years ago

Alright I got it working. Can sort by anything. I'm wondering why we don't enable sorting by name on the course page.

But anyway, the CSS is a mess. I have to clean up the CSS before I can submit this.

btbonval commented 10 years ago

It doesn't look like assets/js/sort-tables.js is being used. Might be worth looking into for cleanup.

btbonval commented 10 years ago

unique class name are probably due to DataTables aoColumnDefs which may be specified by classname amongst other options, but not by id. https://datatables.net/usage/columns

Since I'm already tinkering, I might as well cleanup the CSS to remove empty CSS definitions like these: https://github.com/FinalsClub/karmaworld/blob/c3b37c3a6491de4e4fe3e812e25e3bc53d1a697f/karmaworld/assets/css/course_list_table.css#L59-L63

and replace the class targets with numerically ordered targets.

btbonval commented 10 years ago

Alright I have both course list and course detail looking right and sorting. Now I have to copypasta the javascript and template into search results.

This will work, but it'd be great to refactor all this copypasta I'm making if there's a sensible way to do so. Not sure that there is.

btbonval commented 10 years ago

pushed to master. looks good. closing ticket.

btbonval commented 10 years ago

This change is just one of the things I had wanted to see for #223 , which we decided to close and ignore. But I'm glad to see some of the consistency making its way in!

AndrewMagliozzi commented 10 years ago

Thanks Bryan! More front end fixes coming soon thanks to Perry our UI/UX expert.

On Feb 21, 2014, at 2:06 AM, Bryan Bonvallet notifications@github.com wrote:

This change is just one of the things I had wanted to see for #223 , which we decided to close and ignore. But I'm glad to see some of the consistency making its way in!

— Reply to this email directly or view it on GitHub.