-
Notifications
You must be signed in to change notification settings - Fork 85
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
Browse filter results on page #473
base: main
Are you sure you want to change the base?
Browse filter results on page #473
Conversation
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.
@litvinovg I don't have any big complain on your code, the implementation is aligned with other Vitro DataGetters implementations. Please find a couple of my tiny comments.
The PR feature has been tested and it works as it is described with the exception of person's publications.
api/src/main/java/edu/cornell/mannlib/vitro/webapp/search/controller/PagedSearchController.java
Outdated
Show resolved
Hide resolved
api/src/main/java/edu/cornell/mannlib/vitro/webapp/search/controller/PagedSearchController.java
Outdated
Show resolved
Hide resolved
api/src/main/java/edu/cornell/mannlib/vitro/webapp/search/controller/PagedSearchController.java
Outdated
Show resolved
Hide resolved
.../vitro/webapp/edit/n3editing/configuration/preprocessors/utils/ProcessDataGetterN3Utils.java
Outdated
Show resolved
Hide resolved
...ain/java/edu/cornell/mannlib/vitro/webapp/utils/dataGetter/SearchFilterValuesDataGetter.java
Outdated
Show resolved
Hide resolved
home/src/main/resources/rdf/i18n/de_DE/interface-i18n/firsttime/vitro_UiLabel.ttl
Show resolved
Hide resolved
uil-data:browse_results_alphabetical_index.Vitro | ||
rdf:type owl:NamedIndividual ; | ||
rdf:type uil:UILabel ; | ||
rdfs:label "a,b,c,d,e,f,g,h,i,j,k,l,m,n,o,p,q,r,s,t,u,v,w,x,y,z"@pt-BR ; |
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.
Should check this with a Portuguese native speaker
…r in SearchFilterValuesDataGetter
0e97588
to
af7a82e
Compare
…s with results before values without results. Added :limitDisplayTo object property.
3fc2ad1
to
a180e13
Compare
e69258f
to
2db1ce9
Compare
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.
@litvinovg please check my comments. Moreover, the search filter by publication year requests rebuilding search index in the case some record publication date has been changed? I was expected it might be automatically done in the background.
@@ -14,18 +14,14 @@ public class FilterValue { | |||
|
|||
private boolean isDefaultValue; | |||
|
|||
private boolean publiclyAvailable = true; | |||
private boolean display = false; |
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.
displayed? isDisplayed()? setDisplayed(boolean value)?
|
||
public List<FieldVTwo> retrieveFields(int counter) { | ||
List<FieldVTwo> fields = new ArrayList<FieldVTwo>(); | ||
fields.add(new FieldVTwo().setName(getVarName("filterUri", counter))); |
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.
ProcessSearchFilterValuesDataGetterN3.searchFilterVarBase instead of "filterUri".
private String type = FILTER; | ||
private String rangeText = ""; | ||
private String rangeInput = ""; | ||
private boolean hidden = false; | ||
private boolean display = false; |
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.
displayed
@@ -6,8 +6,7 @@ public class SearchFilterGroup { | |||
|
|||
private String id; | |||
private String label; | |||
private boolean isPublic = false; | |||
private boolean hidden = true; | |||
private boolean display = false; |
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.
displayed
SearchFiltering.addRangeFilter(query, searchFilter); | ||
} | ||
for (FilterValue fv : searchFilter.getValues().values()) { | ||
if (fv.isDefault() || fv.getSelected()) { |
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.
Should we renamed the method taking into account change in the implementation? If I understood well it includes now also selected, not just default filters.
<div id="browsing-options"> | ||
<@showSortOptions /> | ||
<@sl.showHits "filter-form" /> | ||
<img id="downloadIcon" alt="${i18n().download_results}" title="${i18n().download_results}" /> |
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.
Do we need here src attribute? There is no image for download.
VIVO PR
What does this pull request do?
Adds an option to select search filter to display filtered results on custom page.
What's new?
Refactored PagedSearchController.
Added search filter option to ManagePageGenerator
Refactored ProcessN3GetterDataMap
Created new data getter class display:SearchFilterValuesDataGetter , java implementation in SearchFilterValuesDataGetter.java
Created boolean data property search:reverseFacetOrder to reverse sort order of facets.
Created filters to filter result in alphabetical indexes
:filter_label_regex - default regex filter that uses i18n field :field_label_display with name locale + _label_display
:filter_raw_label_regex - fallback regex filter that uses nameRaw search field for VIVO instances without i18n support
search:limitDisplayTo object property was created to limit user access to display of search groups, filters, values.
Resolved bug on search page which appears if user select too many filters. In that case search results are empty and selected filter values are not shown on the page, so the user can't unselect them.
Added options to sort results, number of results on page and "Download results" options on custom pages
How should this be tested?
A description of what steps someone could take to:
Additional Notes:
This change require documentation to be updated.
Interested parties
@hauschke @VIVO-project/vivo-committers
Reviewers' expertise
Candidates for reviewing this PR should have some of the following expertises:
Reviewers' report template
Please update the following template which should be used by reviewers.
General comment
A reviewer should provide here comments and suggestions for requested changes if any.
Testing
A reviewer should briefly describe here how it was tested
Code reviewing
A reviewer should briefly describe here which part was code reviewed