-
Notifications
You must be signed in to change notification settings - Fork 789
AO3-7143 Adding filter to user collection page #5788
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
base: master
Are you sure you want to change the base?
Changes from 29 commits
b3246cd
aae1900
c202951
960ef97
f512be0
466ffa6
cdad56d
89eae21
23a987b
a00bc74
322e4bc
9e610be
c98d152
2ad01c0
6eb7ad8
30e34b7
42163e5
182f9fa
84bfc54
7b65fe4
07776bb
d2a6a0d
ec61299
1fef682
d16b8d6
2da1469
b180115
67f0718
f9a5551
7eaedbe
4a4036f
f2393fd
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -13,47 +13,49 @@ | |||||||||||||||||||||
|
|
||||||||||||||||||||||
| <h3 class="heading"> | ||||||||||||||||||||||
| <% if @collections.empty? %> | ||||||||||||||||||||||
| <%= ts("Sorry, there were no collections found.") %> | ||||||||||||||||||||||
| <%= t(".page_heading.no_results") %> | ||||||||||||||||||||||
| <% else %> | ||||||||||||||||||||||
| <%= search_header @collections, @query, ts("Collection") %> | ||||||||||||||||||||||
| <%= search_header @collections, @query, t(".page_heading.search_header") %> | ||||||||||||||||||||||
| <% end %> | ||||||||||||||||||||||
| </h3> | ||||||||||||||||||||||
| <!--/descriptions--> | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| <!--Subnavigation, sorting and actions--> | ||||||||||||||||||||||
| <h3 class="landmark heading"><%= ts("Navigation") %></h3> | ||||||||||||||||||||||
| <ul class="navigation actions" role="navigation"> | ||||||||||||||||||||||
| <% # Collections and Open Challenges links unless a logged in user viewing own collections page %> | ||||||||||||||||||||||
| <% unless logged_in? && @user && @user == current_user %> | ||||||||||||||||||||||
| <li><%= span_if_current ts("Collections"), collections_path %></li> | ||||||||||||||||||||||
| <li><%= link_to ts("Open Challenges"), list_challenges_collections_path %></li> | ||||||||||||||||||||||
| <% end %> | ||||||||||||||||||||||
| <% if logged_in? %> | ||||||||||||||||||||||
| <% # Logged in user on own collections index gets links for user Collections and Manage Collection Items %> | ||||||||||||||||||||||
| <% if @user && @user == current_user %> | ||||||||||||||||||||||
| <li><%= span_if_current ts("Collections"), user_collections_path(@user) %></li> | ||||||||||||||||||||||
| <li><%= link_to ts("Manage Collection Items"), user_collection_items_path(@user) %></li> | ||||||||||||||||||||||
| <div class="navigation actions module"> | ||||||||||||||||||||||
| <h3 class="landmark heading"><%= t(".navigation.landmark_heading") %></h3> | ||||||||||||||||||||||
| <ul class="navigation actions" role="navigation"> | ||||||||||||||||||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You can remove the |
||||||||||||||||||||||
| <%# Collections and Open Challenges links unless a logged in user viewing own collections page %> | ||||||||||||||||||||||
| <% unless logged_in? && @user && @user == current_user %> | ||||||||||||||||||||||
| <li><%= span_if_current t(".navigation.actions.collections"), collections_path %></li> | ||||||||||||||||||||||
| <li><%= link_to t(".navigation.actions.open_challenges"), list_challenges_collections_path %></li> | ||||||||||||||||||||||
| <% end %> | ||||||||||||||||||||||
| <% # Logged in collection maintainer on own collection gets link for New Subcollection and all other logged in users get New Collection link %> | ||||||||||||||||||||||
| <% if @collection && !@collection.parent && @collection.user_is_maintainer?(current_user) %> | ||||||||||||||||||||||
| <li><%= link_to ts("New Subcollection"), new_collection_collection_path(@collection) %></li> | ||||||||||||||||||||||
| <% else %> | ||||||||||||||||||||||
| <li><%= link_to ts("New Collection"), new_collection_path %></li> | ||||||||||||||||||||||
| <% if logged_in? %> | ||||||||||||||||||||||
| <%# Logged in user on own collections index gets links for user Collections and Manage Collection Items %> | ||||||||||||||||||||||
| <% if @user && @user == current_user %> | ||||||||||||||||||||||
| <li><%= span_if_current t(".navigation.actions.collections"), user_collections_path(@user) %></li> | ||||||||||||||||||||||
| <li><%= link_to t(".navigation.actions.manage_items"), user_collection_items_path(@user) %></li> | ||||||||||||||||||||||
| <% end %> | ||||||||||||||||||||||
| <%# Logged in collection maintainer on own collection gets link for New Subcollection and all other logged in users get New Collection link %> | ||||||||||||||||||||||
| <% if @collection && !@collection.parent && @collection.user_is_maintainer?(current_user) %> | ||||||||||||||||||||||
| <li><%= link_to t(".navigation.actions.new_subcollection"), new_collection_collection_path(@collection) %></li> | ||||||||||||||||||||||
| <% else %> | ||||||||||||||||||||||
| <li><%= link_to t(".navigation.actions.new_collection"), new_collection_path %></li> | ||||||||||||||||||||||
| <% end %> | ||||||||||||||||||||||
| <% end %> | ||||||||||||||||||||||
| <% end %> | ||||||||||||||||||||||
| <% if @sort_and_filter %> | ||||||||||||||||||||||
| <% # Filters button for narrow screens jumps to filters when JavaScript is disabled and opens filters when JavaScript is enabled %> | ||||||||||||||||||||||
| <li class="narrow-shown hidden"><a href="#collection-filters" id="go_to_filters"><%= ts("Filters") %></a></li> | ||||||||||||||||||||||
| <% end %> | ||||||||||||||||||||||
| </ul> | ||||||||||||||||||||||
| <% if @sort_and_filter %> | ||||||||||||||||||||||
| <%# Filters button for narrow screens jumps to filters when JavaScript is disabled and opens filters when JavaScript is enabled %> | ||||||||||||||||||||||
| <li class="narrow-shown hidden"><a href="#collection-filters" id="go_to_filters"><%= t(".navigation.actions.filters") %></a></li> | ||||||||||||||||||||||
| <% end %> | ||||||||||||||||||||||
| </ul> | ||||||||||||||||||||||
| </div> | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| <% unless @collections.blank? %> | ||||||||||||||||||||||
| <!--pagination here--> | ||||||||||||||||||||||
| <%= will_paginate @collections %> | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| <!--main content--> | ||||||||||||||||||||||
| <h3 class="landmark heading"><%= ts("List of Collections") %></h3> | ||||||||||||||||||||||
| <ul class="collection picture index group"> | ||||||||||||||||||||||
| <h3 class="landmark heading"><%= t(".main_content.landmark_heading") %></h3> | ||||||||||||||||||||||
| <ul class="collection picture index group <%= "no-filter" unless @sort_and_filter %>"> | ||||||||||||||||||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Instead of adding a class, let's take a look at what's causing the problem here. (But just a quick note for the future that we have some rules for adding classes.) It looks like all collection index pages get the otwarchive/app/helpers/application_helper.rb Lines 60 to 62 in 7f6d5b1
otwarchive/app/helpers/application_helper.rb Lines 19 to 25 in 7f6d5b1
Unfortunately, collections don't use As a bonus, this approach should also fix the issue where
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I was trying to find the root cause but couldn't quite find it. Thanks! |
||||||||||||||||||||||
| <% @collections.each do |collection| %> | ||||||||||||||||||||||
| <%= render :partial => "collection_blurb", :locals => {:collection => collection} %> | ||||||||||||||||||||||
| <% 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.
We only put our subnavigation in
divlike this in rare instances where we have complex navigation with multiple separate sets of actions, like two lists, or a list and a form outside the list. Thedivonworks/index.html.erbis there because we can have both a form and a list... but I think it's wrong, actually, because they shouldn't be there at the same time.I'm pretty sure the reason you've added this is to make the navigation take up the full width of
#mainand push the filters into the right position on the page. Interestingly, the bookmarks index doesn't have thisdiv, nor does it have that problem -- and after some investigating, I figured out why. Both bookmark and work index pages leave the emptyol.indexon the page, while the collection index removes it with theunless @collections.blank?logic on line 52. Empty list elements aren't great for accessibility, so we don't want to replicate that here, either.I think the solution we need here is to add a
cleartoform.filters. That should keep the filters in the right position without either the empty list or thediv.Uh oh!
There was an error while loading. Please reload this page.
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.
It seems that stylesheet 17 is taking priority over stylesheet 18. Regardless of if the index has filtered or not. The
width: 100%seems to be the cause of the issue in that case? It is defined to be 75% in sheet 18 within line 9 but the one in style sheet 17 is applied first. Adding!importantto the one in sheet 18 seems to fix it? But I wasn't sure if such a solution was desired.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.
Which page and device/browser, and which
width: 100%? After actioning this comment, the onlywidth: 100%I can find in 17 is.dashboard.works-index h4.landmark, .dashboard.works-index ol.pagination, and I'm not running into issues on either the user collection index or work index, but I've only checked in desktop Safari.0c99f89 has the changes I'm testing with.
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.
Oh I see, I got confused and reintroduced the class in page 17.