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

Remove datatables class from All Works display table, which was causi… #3317

Merged
merged 1 commit into from Oct 11, 2018

Conversation

adamjarling
Copy link
Member

…ng double search and pagination

Fixes #3306

This removes the double search and pagination which was appearing in Works "All Items" page.

After:

works-regular-search-pagination

Before:

works-extra-search-pagination

@samvera/hyrax-code-reviewers

@@ -60,9 +60,9 @@
<% elsif current_page?(hyrax.dashboard_works_path(locale: nil)) && !current_ability.admin? %>
<span class="count-display"><%= I18n.t('hyrax.my.count.works.you_manage', total_count: @response.total_count).html_safe %></span>
<% else %>
<span class="count-display"><%= I18n.t('hyrax.my.count.works.in_repo', total_count: @response.total_count).html_safe %></span>
<span class="count-display"><%= I18n.t('hyrax.my.count.works.in_repo', total_count: @response.total_count).html_safe %></span>
Copy link
Member

Choose a reason for hiding this comment

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

Should we drop this whitespace-only diff?

@@ -1,4 +1,4 @@
<table class="table table-striped works-list datatable">
<table class="table table-striped works-list">
Copy link
Member

Choose a reason for hiding this comment

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

This looks like it was added in #3173. I suspect we need a larger rework of this to allow both sorting and pagination.

@samvera/hyrax-ui-ux-advisors, @vantuyls, @bess

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, so datatables was added for sorting, but it (from #3173 PR screenshot), looks like it doubled the UI. Is the solution to rip out the existing pagination and search, and let datatables handle pagination, search, and sorting?

Copy link
Member

Choose a reason for hiding this comment

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

maybe, but i think that is also problematic, because the datatables only work on data in the current page. i think we would also need to change the relevant search builder to make the datatables work over all results. this problem exists elsewhere (e.g. notifications), as well.

the other alternative is to remove datatables and implement our own sorting.

since the original contribution wasn't part of a particular ticket, i'm tempted to suggest we revert it as proposed here and start from scratch. but i'd also like to get @bess (or maybe @mark-dce's) input before moving forward. can either of you comment on the original motivation for this?

@adamjarling adamjarling force-pushed the 3306-remove-extra-pagination-from-dashboard-works branch from ab22d36 to 2397f87 Compare October 5, 2018 16:50
Copy link
Member

@no-reply no-reply left a comment

Choose a reason for hiding this comment

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

We discussed this in a Hyrax WG meeting and decided revert and start from scratch is the best approach.

@no-reply no-reply merged commit cef0b72 into master Oct 11, 2018
Hyrax WG -- Sprint 4 automation moved this from Review to Done Oct 11, 2018
@no-reply no-reply deleted the 3306-remove-extra-pagination-from-dashboard-works branch October 11, 2018 20:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

3 participants