Skip to content
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

show back to search and start over for 1 result show pages #1065

Merged
merged 1 commit into from
Jan 29, 2015

Conversation

mejackreed
Copy link
Contributor

This pull request modifies a show page to include the "Back to Search" and "Start Over" buttons from a navigated search that only includes 1 search result. Previously both the pagination and search buttons were both inside #previousNextDocument and only shown if @previous_document || @next_document. I think this logic should be decoupled and that these are separate concerns.

GeoBlacklight usability testing indicated not showing "Back to Search" and "Start Over" on every show page navigated from a search was confusing for users. It also was not clear to users why this was happening.

@@ -1,18 +1,19 @@
<% #Using the Bootstrap Pagination class -%>
<% if @previous_document || @next_document %>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you fix the indentation in this file? it seems a little screwy (and probably not really your fault.)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

screwy for sure... force pushed

<% end %>
<%= link_back_to_catalog :class => 'btn' %>

<%=link_to "#{t('blacklight.search.start_over')}", start_over_path(current_search_session.try(:query_params) || {}), :id=>"startOverLink", :class => 'btn' %>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While you're touching this file... want to change this line:

<%= link_to t('blacklight.search.start_over'), start_over_path(current_search_session.try(:query_params) || {}), id: "startOverLink", class: 'btn'  %>

@jcoyne
Copy link
Member

jcoyne commented Jan 27, 2015

Yes, this is a good idea.

@jrochkind
Copy link
Member

Changing the HTML DOM id from #previousNextDocument to #paginationSearchWidgets will be a backwards compatibility issue for anyone with local CSS styling using that HTML id. (I do!).

If you're going to go all the way to backwards incompat -- is there any reason this is styled with an id instead of a class anyway? Using an id means there can only be one per page without it being technically illegal HTML, conceivably someone might sometimes want more than one on a page (top and bottom or something).

Change to a class?

Or, hey, best of both worlds -- leave the id the same, #previousNextDocument for backwards compat, but add a class .pagination-search-widgets, BL CSS will reference the class not the id. ID still there for backwards compat, but BL uses the better name, applied as a class not an ID.

?

@mejackreed
Copy link
Contributor Author

@jrochkind I'm ok with your suggestion for backwards compatibility, with a comment to remove id selectors in Blacklight 6

@mejackreed
Copy link
Contributor Author

Ok so now updated with @jrochkind s suggestion for backward compatibility with deprecation notice.

@@ -1,18 +1,20 @@
<% #Using the Bootstrap Pagination class -%>
<% if @previous_document || @next_document %>
<div id="previousNextDocument">
<% Deprecation.warn('', 'using id="previousNextDocument" as a selector is deprecated and will be removed in Blacklight 6.0') %>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this unconditional deprecation warning will get annoying really fast. Can we either leave a comment (or comment out the block) instead, and remember to include it in release notes when we kill the id?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cbeer added as a comment

@cbeer
Copy link
Member

cbeer commented Jan 28, 2015

👍

jcoyne added a commit that referenced this pull request Jan 29, 2015
show back to search and start over for 1 result show pages
@jcoyne jcoyne merged commit f838d0a into master Jan 29, 2015
@jcoyne jcoyne deleted the results-bar branch January 29, 2015 13:29
@cbeer cbeer modified the milestone: 5.9.0 Jan 31, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants