-
Notifications
You must be signed in to change notification settings - Fork 282
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
HEX-561: Add sort by recent downloads feature #566
HEX-561: Add sort by recent downloads feature #566
Conversation
Add new ninety_days view to package_downloads view Add new migration to drop and reinsert package_downloads MATERIALIZED VIEW Add ninety_days sort to package.ex Add function to query db for all views within a list of packages in package_download.ex Update default sorting to ninety_days in: package_controller, layout/header.html.eex, opensearch.xml.eex, page/index.html.eex Update OpenSearchController test to include ninety_days sort by default change Update :downloads atom to :total_downloads Add tooltip to downloads info css Update _package.html.eex template to display appropriate downloads desc text for current sort method Update _package.html.eex template to display a tooltip, when hovering over downloads desc text, with either total downloads or recent downloads depending upon sort method Update show_sort_info functions to include sort by changes (i.e. total downloads or ninety_days). This update fixed a bug where the show_sort_info wasn't displaying the text as the functions expected a string while the actual input was coming in as an atom Update PackageViewTest to reflect show sort info changes Update seed.exs with inclusions of ninety_day downloads for packages: decimal, ecto, postgrex, and ups packages Update package_test.exs to include :total_downloads sort change
Hello, @jeramyRR! This is your first Pull Request that will be reviewed by Ebert, an automatic Code Review service. It will leave comments on this diff with potential issues and style violations found in the code as you push new commits. You can also see all the issues found on this Pull Request on its review page. Please check our documentation for more information. |
-webkit-transition: all 0.2s ease; | ||
-moz-transition: all 0.2s ease; | ||
-o-transition: all 0.2s ease; | ||
transition: all 0.2s ease; |
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.
0.2
should be written without a leading zero as .2
top: 1.2em; | ||
-webkit-transition: all 0.2s ease; | ||
-moz-transition: all 0.2s ease; | ||
-o-transition: all 0.2s ease; |
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.
0.2
should be written without a leading zero as .2
top: 1.2em; | ||
-webkit-transition: all 0.2s ease; | ||
-moz-transition: all 0.2s ease; | ||
-o-transition: all 0.2s ease; |
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.
Avoid vendor prefixes.
visibility: visible; | ||
top: 1.2em; | ||
-webkit-transition: all 0.2s ease; | ||
-moz-transition: all 0.2s ease; |
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.
0.2
should be written without a leading zero as .2
visibility: visible; | ||
top: 1.2em; | ||
-webkit-transition: all 0.2s ease; | ||
-moz-transition: all 0.2s ease; |
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.
Avoid vendor prefixes.
.downloads-count-wrapper:hover .tooltiptext { | ||
visibility: visible; | ||
top: 1.2em; | ||
-webkit-transition: all 0.2s ease; |
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.
0.2
should be written without a leading zero as .2
.downloads-count-wrapper:hover .tooltiptext { | ||
visibility: visible; | ||
top: 1.2em; | ||
-webkit-transition: all 0.2s ease; |
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.
Avoid vendor prefixes.
} | ||
} | ||
.downloads-count-wrapper:hover .tooltiptext { | ||
visibility: visible; |
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.
Properties should be ordered top, visibility, transition
transition: all 0.3s ease-in-out; | ||
} | ||
} | ||
.downloads-count-wrapper:hover .tooltiptext { |
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.
Selector should have depth of applicability no greater than 4, but was 6.
Nested selectors are highly coupled to the DOM structure, they will produce less efficient selectors and they compromises the readability of your code although it reduces the duplication of parent selectors. It is recommended to follow the inception rule in such scenarios.
You can reduce this nesting by introducing new selectors targeting the nested elements instead of relying in their parent / child dependency.
-o-transition: all 0.3s ease; | ||
transition: all 0.3s ease-in-out; | ||
} | ||
} |
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.
Rule declaration should be followed by an empty line
-webkit-transition: all 0.3s ease; | ||
-moz-transition: all 0.3s ease-in-out; | ||
-o-transition: all 0.3s ease; | ||
transition: all 0.3s ease-in-out; |
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.
0.3
should be written without a leading zero as .3
text-transform: none; | ||
-webkit-transition: all 0.3s ease; | ||
-moz-transition: all 0.3s ease-in-out; | ||
-o-transition: all 0.3s ease; |
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.
0.3
should be written without a leading zero as .3
text-transform: none; | ||
-webkit-transition: all 0.3s ease; | ||
-moz-transition: all 0.3s ease-in-out; | ||
-o-transition: all 0.3s ease; |
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.
Avoid vendor prefixes.
top: -2em; | ||
text-transform: none; | ||
-webkit-transition: all 0.3s ease; | ||
-moz-transition: all 0.3s ease-in-out; |
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.
0.3
should be written without a leading zero as .3
top: -2em; | ||
text-transform: none; | ||
-webkit-transition: all 0.3s ease; | ||
-moz-transition: all 0.3s ease-in-out; |
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.
Avoid vendor prefixes.
position: relative; | ||
top: -2em; | ||
text-transform: none; | ||
-webkit-transition: all 0.3s ease; |
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.
0.3
should be written without a leading zero as .3
position: relative; | ||
top: -2em; | ||
text-transform: none; | ||
-webkit-transition: all 0.3s ease; |
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.
Avoid vendor prefixes.
text-align: center; | ||
border-radius: 4px; | ||
border: #919ca2 1px solid; | ||
padding: 5px 5px; |
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.
Shorthand form for property padding
should be written more concisely as 5px
instead of 5px 5px
} | ||
.tooltiptext { | ||
@include font-size(14, $base-line-height); | ||
visibility: hidden; |
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.
Properties should be ordered position, top, width, padding, color, text-align, text-transform, visibility, background-color, border, border-radius, transition
.downloads-count-desc { | ||
border-bottom: 1px dotted $color-purple; | ||
} | ||
.tooltiptext { |
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.
Selector should have depth of applicability no greater than 4, but was 6.
Nested selectors are highly coupled to the DOM structure, they will produce less efficient selectors and they compromises the readability of your code although it reduces the duplication of parent selectors. It is recommended to follow the inception rule in such scenarios.
You can reduce this nesting by introducing new selectors targeting the nested elements instead of relying in their parent / child dependency.
} | ||
|
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.
Line contains trailing whitespace
} | ||
} | ||
|
||
.downloads-count-wrapper:hover .tooltiptext { |
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.
Selector should have depth of applicability no greater than 4, but was 6.
Nested selectors are highly coupled to the DOM structure, they will produce less efficient selectors and they compromises the readability of your code although it reduces the duplication of parent selectors. It is recommended to follow the inception rule in such scenarios.
You can reduce this nesting by introducing new selectors targeting the nested elements instead of relying in their parent / child dependency.
|
||
.downloads-count-desc { | ||
border-bottom: 1px dotted $color-purple; | ||
} |
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.
Rule declaration should be followed by an empty line
position: relative; | ||
display: inline-block; | ||
|
||
.downloads-count-desc { |
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.
Selector should have depth of applicability no greater than 4, but was 6.
Nested selectors are highly coupled to the DOM structure, they will produce less efficient selectors and they compromises the readability of your code although it reduces the duplication of parent selectors. It is recommended to follow the inception rule in such scenarios.
You can reduce this nesting by introducing new selectors targeting the nested elements instead of relying in their parent / child dependency.
@@ -93,8 +93,40 @@ | |||
font-weight: bold; | |||
color: $color-text-grey; | |||
} | |||
.downloads-count-wrapper { |
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.
Selector should have depth of applicability no greater than 4, but was 5.
Nested selectors are highly coupled to the DOM structure, they will produce less efficient selectors and they compromises the readability of your code although it reduces the duplication of parent selectors. It is recommended to follow the inception rule in such scenarios.
You can reduce this nesting by introducing new selectors targeting the nested elements instead of relying in their parent / child dependency.
assert [^phoenix_id, ^ecto_id] = Package.all([repository], 1, 10, nil, :total_downloads, nil) |> Repo.pluck(:id) | ||
end | ||
|
||
|
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.
There should be no more than 1 consecutive blank lines.
lib/hexpm/web/views/package_view.ex
Outdated
Map.get(downloads, package.id, %{"all" => 0, "ninety_days" => 0}) | ||
end | ||
|
||
|
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.
There should be no more than 1 consecutive blank lines.
lib/hexpm/repository/packages.ex
Outdated
|> Repo.all() | ||
|> Enum.into([]) | ||
|> Enum.reduce(%{}, fn(package, acc) -> | ||
{ id, view, dls } = package |
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.
There is no whitespace around parentheses/brackets most of the time, but here there is.
lib/hexpm/repository/packages.ex
Outdated
|> Map.put(view, dls) | ||
Map.update!(acc, id, fn(_) -> map end) | ||
false -> | ||
Map.put_new(acc, id, %{ view => dls }) |
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.
There is no whitespace around parentheses/brackets most of the time, but here there is.
lib/hexpm/repository/package.ex
Outdated
order_by: [fragment("? DESC NULLS LAST", d.downloads)]) | ||
end | ||
|
||
|
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.
There should be no more than 1 consecutive blank lines.
@@ -93,8 +93,43 @@ | |||
font-weight: bold; | |||
color: $color-text-grey; | |||
} | |||
|
|||
.downloads-count-wrapper { |
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.
Selector should have depth of applicability no greater than 4, but was 5.
Nested selectors are highly coupled to the DOM structure, they will produce less efficient selectors and they compromises the readability of your code although it reduces the duplication of parent selectors. It is recommended to follow the inception rule in such scenarios.
You can reduce this nesting by introducing new selectors targeting the nested elements instead of relying in their parent / child dependency.
@@ -2,7 +2,7 @@ defmodule Hexpm.Web.PackageController do | |||
use Hexpm.Web, :controller | |||
|
|||
@packages_per_page 30 | |||
@sort_params ~w(name downloads inserted_at updated_at) | |||
@sort_params ~w(name ninety_days total_downloads inserted_at updated_at) |
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.
ninety_days is fine for internal use, but lets call it recent_downloads and total_downloads for users.
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.
Updated to recent_downloads.
lib/hexpm/repository/packages.ex
Outdated
def packages_downloads_with_all_views(packages) do | ||
PackageDownload.packages_and_all_download_views(packages) | ||
|> Repo.all() | ||
|> Enum.into([]) |
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.
Repo.all()
already returns a list.
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.
Removed unnecessary Enum.into.
lib/hexpm/repository/packages.ex
Outdated
|> Enum.into([]) | ||
|> Enum.reduce(%{}, fn(package, acc) -> | ||
{id, view, dls} = package | ||
case Map.has_key?(acc, id) do |
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.
Using Map.update/4
should simplify this.
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.
Thanks! Using Map.update/4 reduced this peace into two or three lines.
join: p in assoc(pd, :package), | ||
where: pd.package_id in ^package_ids, | ||
select: {p.id, pd.view, coalesce(pd.downloads, 0)}, | ||
order_by: [desc: pd.view]) |
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.
order_by
is not needed since we will put it in a map later, right?
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.
Removed order_by.
<div class="downloads-count-wrapper"> | ||
<span class="downloads-count-desc"><%= display_downloads_view_title(@view) %></span><br> | ||
<span class="tooltiptext"><%= display_downloads_for_opposite_views(@package_downloads, @view) %></span> | ||
</div> |
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.
Can you show screenshots of the UI changes?
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.
Added below.
Remove order_by from packages_and_all_download_views function Simplify the Map.update in packages_downloads_with_all_views function
lib/hexpm/web/views/package_view.ex
Outdated
|
||
def display_downloads(package_downloads, view) do | ||
case view do | ||
:recent_downloads-> |
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.
There are spaces around operators most of the time, but not here.
lib/hexpm/web/views/package_view.ex
Outdated
|
||
def display_downloads_for_opposite_views(package_downloads, view) do | ||
case view do | ||
:recent_downloads-> |
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.
There are spaces around operators most of the time, but not here.
lib/hexpm/web/views/package_view.ex
Outdated
|
||
def display_downloads_view_title(view) do | ||
case view do | ||
:recent_downloads-> "recent downloads" |
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.
There are spaces around operators most of the time, but not here.
I still have to figure out how to properly create a test method for this. The current one is just a copy paste, and it's failing. |
Update package_view_test and opensearch_controller_test to reflect ninety_days to recent_downloads change
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 added a few minor comments but this looks good to me.
lib/hexpm/repository/packages.ex
Outdated
The return value is a map with the following structure: | ||
%{ package.id => %{ view1 => numDownloads, view2 => numDownloads, ...} | ||
Example: %{ 105 => %{ "all" => 1_234_567, "recent" => 10_000, etc... } | ||
""" |
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.
We generally do not add docs here.
|
||
def index(conn, params) do | ||
# TODO: Handle /repos/:repo/ and / | ||
repositories = Users.all_repositories(conn.assigns.current_user) | ||
page = Hexpm.Utils.safe_int(params["page"]) | ||
search = Hexpm.Utils.parse_search(params["search"]) | ||
sort = Hexpm.Utils.safe_to_atom(params["sort"] || "name", @sort_params) | ||
sort = Hexpm.Utils.safe_to_atom(params["sort"] || "recent_downloads", @sort_params) |
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 don't think we should change the default sorting here.
sort params: | ||
recent_downloads is defined as the sum of a packages downloads over ninety days. | ||
total_downloads is the sum of all a packages downloads. | ||
""" |
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.
No docs here either.
@@ -20,12 +26,12 @@ defmodule Hexpm.Web.PackageController do | |||
end | |||
|
|||
repositories = Users.all_repositories(conn.assigns.current_user) | |||
sort = Hexpm.Utils.safe_to_atom(params["sort"] || "name", @sort_params) | |||
sort = Hexpm.Utils.safe_to_atom(params["sort"] || "recent_downloads", @sort_params) |
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.
Changing the default here is fine 👍
@@ -89,7 +95,7 @@ defmodule Hexpm.Web.PackageController do | |||
|
|||
downloads = Packages.package_downloads(package) | |||
owners = Owners.all(package, [:emails]) | |||
dependants = Packages.search(repositories, 1, 20, "depends:#{package.name}", :downloads, [:name, :repository_id]) | |||
dependants = Packages.search(repositories, 1, 20, "depends:#{package.name}", :total_downloads, [:name, :repository_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.
Use recent_downloads here imo.
@@ -37,7 +37,7 @@ | |||
<form class="navbar-form pull-left" role="search" action="<%= package_path(Endpoint, :index) %>"> | |||
<div class="input-group"> | |||
<input placeholder="Find packages" name="search" type="text" autofocus class="form-control" value="<%= search(assigns) %>"> | |||
<input type="hidden" name="sort" value="downloads"> | |||
<input type="hidden" name="sort" value="recent_downloads"> |
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.
👍
Remove docs from packages.ex and package_controller.ex Return default sort of api/package_controller to name Update default sort of dependants to recent_downloads to match package default sort
Pushed a new commit up to address your comments. |
Thank you, this is a really good improvement! |
Thanks, and thanks for letting me work the feature. This was my first open source contribution, as well as pretty much my first serious Elixir thing, even though it was really small. |
I just deployed this and I love it already. Thank you @jeramyRR. |
Add new ninety_days view to package_downloads view
Add new migration to drop and reinsert package_downloads MATERIALIZED
VIEW
Add ninety_days sort to package.ex
Add function to query db for all views within a list of packages in
package_download.ex
Update default sorting to ninety_days in: package_controller,
layout/header.html.eex, opensearch.xml.eex, page/index.html.eex
Update OpenSearchController test to include ninety_days sort by default
change
Update :downloads atom to :total_downloads
Add tooltip to downloads info css
Update _package.html.eex template to display appropriate downloads desc
text for current sort method
Update _package.html.eex template to display a tooltip, when hovering
over downloads desc text, with either total downloads or recent
downloads depending upon sort method
Update show_sort_info functions to include sort by changes (i.e. total
downloads or ninety_days). This update fixed a bug where the
show_sort_info wasn't displaying the text as the functions expected a
string while the actual input was coming in as an atom
Update PackageViewTest to reflect show sort info changes
Update seed.exs with inclusions of ninety_day downloads for packages:
decimal, ecto, postgrex, and ups packages
Update package_test.exs to include :total_downloads sort change