From 68e7a351b885a00caad5efcbfba45200c0116338 Mon Sep 17 00:00:00 2001
From: Jack Lee <43303581+jacklee1792@users.noreply.github.com>
Date: Fri, 1 Dec 2023 10:55:26 -0500
Subject: [PATCH] Improve ranking links in user profile (#8403)

Links to the rankings page are put on the NR/CR/WR numbers instead, and redirect
to their respective region-based rankings (national, continental, world). When
one of these links is clicked, the "focus" parameter highlights the user's row
in the rankings if applicable and scrolls it to the center of the viewport.
---
 .../app/assets/stylesheets/results.scss       |  4 +++
 .../app/controllers/persons_controller.rb     |  2 ++
 WcaOnRails/app/helpers/persons_helper.rb      | 14 ++++++--
 WcaOnRails/app/helpers/results_helper.rb      |  5 +++
 .../views/persons/_personal_records.html.erb  | 20 +++++------
 .../views/results/_rankings_table.html.erb    |  2 +-
 .../views/results/_results_selector.html.erb  | 35 +++++++++----------
 7 files changed, 48 insertions(+), 34 deletions(-)

diff --git a/WcaOnRails/app/assets/stylesheets/results.scss b/WcaOnRails/app/assets/stylesheets/results.scss
index 58a081aae0..fae4173f67 100644
--- a/WcaOnRails/app/assets/stylesheets/results.scss
+++ b/WcaOnRails/app/assets/stylesheets/results.scss
@@ -49,4 +49,8 @@
   .cubing-icon {
     vertical-align: baseline;
   }
+
+  .highlight-focus {
+    background-color: $highlight-row-color;
+  }
 }
diff --git a/WcaOnRails/app/controllers/persons_controller.rb b/WcaOnRails/app/controllers/persons_controller.rb
index 32dd865a8f..cd19b1b506 100644
--- a/WcaOnRails/app/controllers/persons_controller.rb
+++ b/WcaOnRails/app/controllers/persons_controller.rb
@@ -28,6 +28,8 @@ def index
 
   def show
     @person = Person.current.includes(:user, :ranksSingle, :ranksAverage, :competitions).find_by_wca_id!(params[:id])
+    @country = @person.country
+    @continent = @country.continent
     @previous_persons = Person.where(wca_id: params[:id]).where.not(subId: 1).order(:subId)
     @ranks_single = @person.ranksSingle.select { |r| r.event.official? }
     @ranks_average = @person.ranksAverage.select { |r| r.event.official? }
diff --git a/WcaOnRails/app/helpers/persons_helper.rb b/WcaOnRails/app/helpers/persons_helper.rb
index 4e0f84f8a6..f88a7e1e8e 100644
--- a/WcaOnRails/app/helpers/persons_helper.rb
+++ b/WcaOnRails/app/helpers/persons_helper.rb
@@ -1,10 +1,18 @@
 # frozen_string_literal: true
 
 module PersonsHelper
-  def rank_td(rank_object, type)
-    rank = rank_object&.public_send("#{type}_rank")
+  def rank_td(rank_object, type, wca_id, region_id = nil)
+    region_type = "world"
+    if region_id&.starts_with?("_")
+      region_type = "continent"
+    elsif region_id
+      region_type = "country"
+    end
+    rank = rank_object&.public_send("#{region_type}_rank")
     rank = "-" if rank == 0
-    content_tag :td, rank, class: "#{type}-rank #{'record' if rank == 1}"
+    content_tag(:td, class: "#{region_type}-rank #{'record' if rank == 1}") do
+      rank ? link_to(rank, rankings_path(rank_object.event_id, type, region: region_id, focus: wca_id), class: :plain) : nil
+    end
   end
 
   def odd_rank_reason
diff --git a/WcaOnRails/app/helpers/results_helper.rb b/WcaOnRails/app/helpers/results_helper.rb
index 9fa9883d20..5fd8487ddb 100644
--- a/WcaOnRails/app/helpers/results_helper.rb
+++ b/WcaOnRails/app/helpers/results_helper.rb
@@ -106,6 +106,11 @@ def pb_type_class_for_result(regional_record, pb_marker)
     record_class
   end
 
+  def result_params_merge(**params)
+    # We should unset the focus every time we change the selectors
+    request.params.merge(**params, focus: nil)
+  end
+
   def execute_cached_query(cache_key, sql_query)
     # As we are using the native Rails cache we set the expiry date to 7 days
     # as CAD is ran usually before that
diff --git a/WcaOnRails/app/views/persons/_personal_records.html.erb b/WcaOnRails/app/views/persons/_personal_records.html.erb
index d37ec3a02f..1a5e29f1cf 100644
--- a/WcaOnRails/app/views/persons/_personal_records.html.erb
+++ b/WcaOnRails/app/views/persons/_personal_records.html.erb
@@ -24,22 +24,18 @@
               <%= cubing_icon rank_single.event.id %>
               <%= t "events.#{rank_single.event.id}" %>
             </td>
-            <%= rank_td rank_single, "country" %>
-            <%= rank_td rank_single, "continent" %>
-            <%= rank_td rank_single, "world" %>
+            <%= rank_td rank_single, "single", @person.wca_id, @country.id %>
+            <%= rank_td rank_single, "single", @person.wca_id, @continent.id %>
+            <%= rank_td rank_single, "single", @person.wca_id%>
             <td class="single">
-              <%= link_to rankings_path(rank_single.event_id, "single"), class: "plain" do %>
-                <%= rank_single.solve_time.clock_format %>
-              <% end %>
+              <%= rank_single.solve_time.clock_format %>
             </td>
             <td class="average">
-              <%= link_to rankings_path(rank_single.event_id, "average"), class: "plain" do %>
-                <%= rank_average&.solve_time&.clock_format %>
-              <% end %>
+              <%= rank_average&.solve_time&.clock_format %>
             </td>
-            <%= rank_td rank_average, "world" %>
-            <%= rank_td rank_average, "continent" %>
-            <%= rank_td rank_average, "country" %>
+            <%= rank_td rank_average, "average", @person.wca_id %>
+            <%= rank_td rank_average, "average", @person.wca_id, @continent.id %>
+            <%= rank_td rank_average, "average", @person.wca_id, @country.id %>
             <td><%= odd_rank_reason if odd_rank_reason_needed?(rank_single, rank_average) %></td>
           </tr>
         <% end %>
diff --git a/WcaOnRails/app/views/results/_rankings_table.html.erb b/WcaOnRails/app/views/results/_rankings_table.html.erb
index 4db989cbfd..9c6835fb73 100644
--- a/WcaOnRails/app/views/results/_rankings_table.html.erb
+++ b/WcaOnRails/app/views/results/_rankings_table.html.erb
@@ -25,7 +25,7 @@
       <% rank = value == previous_value ? previous_rank : i+1 %>
       <% tied_previous = rank == previous_rank %>
 
-      <tr>
+      <tr class="<%= params[:focus] == result.personId ? "highlight-focus" : "" %>">
         <td class="pos <%= tied_previous ? "tied-previous" : "" %>"> <%= rank %> </td>
         <td class="name"> <%= link_to result.personName, person_path(result.personId) %> </td>
         <td class="result"> <%= SolveTime.new(params[:event_id], @is_average ? :average : :single, value).clock_format %> </td>
diff --git a/WcaOnRails/app/views/results/_results_selector.html.erb b/WcaOnRails/app/views/results/_results_selector.html.erb
index 8694a69b30..e32c8e6041 100644
--- a/WcaOnRails/app/views/results/_results_selector.html.erb
+++ b/WcaOnRails/app/views/results/_results_selector.html.erb
@@ -7,14 +7,14 @@
   <%= label_tag(t("results.selector_elements.events_selector.event")) %>
   <div id="event">
     <% if show_records_options %>
-      <a href="<%= url_for(request.params.merge(event_id: "all events")) %>"><span class="event-radio all-events <%= (params[:event_id] == "all events" ? " active" : "") %>"
+      <a href="<%= url_for(result_params_merge(event_id: "all events")) %>"><span class="event-radio all-events <%= (params[:event_id] == "all events" ? " active" : "") %>"
                                                                                    id="cubing-icon" data-toggle="tooltip" data-placement="top"
                                                                                    title="<%= t("results.selector_elements.events_selector.all_events") %>">ALL</span></a>
     <% end %>
     <% Event.official.each do |event| %>
       <span class="event-radio">
         <%= link_to cubing_icon(event.id),
-                    request.params.merge(event_id: event.id),
+                    result_params_merge(event_id: event.id),
                     class: (event.id == params[:event_id] ? " active" : ""),
                     id: "cubing-icon",
                     data: {
@@ -40,7 +40,7 @@
     <div id="type" class="btn-group">
       <% @types.each do |type| %>
         <%= link_to t("results.selector_elements.type_selector.#{type}"),
-                      request.params.merge(type: type),
+                      result_params_merge(type: type),
                       class: "btn btn-primary" + (type == params[:type] ? " active" : "") %>
       <% end %>
     </div>
@@ -50,11 +50,11 @@
 <div class="form-group">
   <%= label_tag(t("results.selector_elements.years_selector.years")) %>
   <div id="years" class="btn-group">
-    <%= link_to t("results.selector_elements.years_selector.all_years"), request.params.merge(years: "all years"), class: "btn btn-info" + (@is_all_years ? " active" : "") %>
+    <%= link_to t("results.selector_elements.years_selector.all_years"), result_params_merge(years: "all years"), class: "btn btn-info" + (@is_all_years ? " active" : "") %>
     <div class="btn-group">
       <ul class="dropdown-menu years-only">
         <% @years.each do |year| %>
-          <li title="<%= t('results.tooltip_no_years') %>" class="disabled only <%= (@is_only && year.to_s == @year ? " active" : "") %>"><%= link_to year.to_s, request.params.merge(years: "only #{year}") %></li>
+          <li title="<%= t('results.tooltip_no_years') %>" class="disabled only <%= (@is_only && year.to_s == @year ? " active" : "") %>"><%= link_to year.to_s, result_params_merge(years: "only #{year}") %></li>
         <% end %>
       </ul>
       <label id="only-years" class="btn btn-info <%= @is_only ? " active" : "" %>" data-toggle="dropdown">
@@ -70,7 +70,7 @@
     <div class="btn-group">
       <ul class="dropdown-menu years-until">
         <% @years.each do |year| %>
-          <li title="<%= t('results.tooltip_no_years') %>" class="disabled until <%= (@is_until && year.to_s == @year ? " active" : "") %>"><%= link_to year.to_s, request.params.merge(years: "until #{year}") %></li>
+          <li title="<%= t('results.tooltip_no_years') %>" class="disabled until <%= (@is_until && year.to_s == @year ? " active" : "") %>"><%= link_to year.to_s, result_params_merge(years: "until #{year}") %></li>
         <% end %>
       </ul>
       <label id="until-years" class="btn btn-info <%= @is_until ? " active" : "" %>" data-toggle="dropdown">
@@ -89,9 +89,9 @@
 <div class="form-group">
   <%= label_tag(t("results.selector_elements.gender_selector.gender")) %>
   <div id="gender" class="btn-group">
-    <%= link_to t("results.selector_elements.gender_selector.gender_all"), request.params.merge(gender: "All"), class: "btn btn-info" + (@gender == "All" ? " active" : "") %>
-    <%= link_to t("results.selector_elements.gender_selector.male"), request.params.merge(gender: "Male"), class: "btn btn-info" + (@gender == "Male" ? " active" : "") %>
-    <%= link_to t("results.selector_elements.gender_selector.female"), request.params.merge(gender: "Female"), class: "btn btn-info" + (@gender == "Female" ? " active" : "") %>
+    <%= link_to t("results.selector_elements.gender_selector.gender_all"), result_params_merge(gender: "All"), class: "btn btn-info" + (@gender == "All" ? " active" : "") %>
+    <%= link_to t("results.selector_elements.gender_selector.male"), result_params_merge(gender: "Male"), class: "btn btn-info" + (@gender == "Male" ? " active" : "") %>
+    <%= link_to t("results.selector_elements.gender_selector.female"), result_params_merge(gender: "Female"), class: "btn btn-info" + (@gender == "Female" ? " active" : "") %>
   </div>
 </div>
 
@@ -111,7 +111,7 @@
         </label>
         <ul class="dropdown-menu show-persons">
           <% @quantities.each do |quantity| %>
-            <li title="<%= (quantity.to_s != @quantities[0] ? t('results.tooltip_no_quantities') : "") %>" class="persons <%= (@is_persons && quantity.to_s == @show ? " active" : "") %> <%= (quantity.to_s != @quantities[0] ? " disabled" : "") %>"><%= link_to quantity.to_s, request.params.merge(show: "#{quantity} persons") %></li>
+            <li title="<%= (quantity.to_s != @quantities[0] ? t('results.tooltip_no_quantities') : "") %>" class="persons <%= (@is_persons && quantity.to_s == @show ? " active" : "") %> <%= (quantity.to_s != @quantities[0] ? " disabled" : "") %>"><%= link_to quantity.to_s, result_params_merge(show: "#{quantity} persons") %></li>
           <% end %>
         </ul>
       </div>
@@ -129,12 +129,12 @@
           <% @quantities.each do |quantity| %>
             <%# querying more than 100 single ranking rows fries our DB, so we block it both here and in the controller until we figure out a more permanent solution %>
               <li title="<%= (quantity.to_s != @quantities[0] ? t('results.tooltip_no_quantities') : "") %>" class="results <%= (@is_results && quantity.to_s == @show ? " active" : "") %> <%= (quantity.to_s != @quantities[0] ? " disabled" : "") %>">
-                <%= link_to quantity.to_s, request.params.merge(show: "#{quantity} results")  %>
+                <%= link_to quantity.to_s, result_params_merge(show: "#{quantity} results")  %>
               </li>
           <% end %>
         </ul>
       </div>
-      <%= link_to t("results.selector_elements.show_selector.by_region"), request.params.merge(show: "by region"), class: "btn btn-info" + (@is_by_region ? " active" : "") %>
+      <%= link_to t("results.selector_elements.show_selector.by_region"), result_params_merge(show: "by region"), class: "btn btn-info" + (@is_by_region ? " active" : "") %>
     </div>
   </div>
 <% elsif show_records_options %>
@@ -143,7 +143,7 @@
     <div id="show" class="btn-group">
       <% @shows.each do |show| %>
         <%= link_to t("results.selector_elements.show_selector.#{show.tr(" ", "_")}"),
-                      request.params.merge(show: show),
+                      result_params_merge(show: show),
                       class: "btn btn-primary" + (show == params[:show] ? " active" : "") %>
       <% end %>
     </div>
@@ -152,10 +152,9 @@
 
 <script>
   $(function() {
-    $('#region').bind('change', function () {
-      var params = window.wca.getUrlParams();
-      params.region = $(this).val();
-      window.location.search = $.param(params);
-    });
+    const highlighted = document.getElementsByClassName("highlight-focus");
+    if (highlighted.length > 0) {
+      highlighted[0].scrollIntoView({ block: "center" });
+    }
   });
 </script>