search

freedomofpress / securethenews

An automated scanner and web dashboard for tracking TLS deployment across news organizations
https://securethe.news
GNU Affero General Public License v3.0
100 stars 29 forks source link

Sort leaderboard by name #290

Closed chigby closed 3 years ago

chigby commented 3 years ago

Fixes #278

This pull request changes the sorting of the leaderboard to sort by news organization name after sorting by the score.

I've chosen to add lodash as a dependency here since its sorting capabilities are more flexible and more in line with the kind of sorting we want to perform here. This doesn't even really add that much to the "weight" of our JS. According to the documentation, if we just import the single function we want, it will only even add the small subset of lodash code that we're using into the final bundle.

chigby commented 3 years ago

Yeah, I suppose not considering articles when sorting by name is maybe a question for @redshiftzero. Initially I didn't think about it, but when I was scrolling through locally, I noticed all the organizations with "The" at the beginning were next to each other. I think it depends on how we think people want to use the list and if, for example, "The New York Times" should be under "T" or "N".

redshiftzero commented 3 years ago

nice, I think removing articles is a good choice since we have so many news organizations beginning with The (and this seems to be a standard approach)

SaptakS commented 3 years ago

@chigby I have resolved the merge conflict modifying my code for sorting by different attributes based on your changes in the PR. Please review and check if looks good. If seems good to you, can merge then

chigby commented 3 years ago

@SaptakS Your change looks great, though while I was looking at it locally I realized there was a problem with my title sorting regular expression. I fixed it in https://github.com/freedomofpress/securethenews/pull/290/commits/ae45db103523ff579ec6ed384a0518ee5284a375 but if you could give it a quick check to make sure, then we can get this merged.

SaptakS commented 3 years ago

Regex fix looks good. Nice catch!