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

Fixes #3178 - Add Select All functionality to Manage Expired Embargo … #3246

Merged
merged 1 commit into from
Sep 11, 2018

Conversation

hweng
Copy link
Contributor

@hweng hweng commented Sep 7, 2018

…table.

Fixes #3178 ; refs #3178

Here is the screenshots for after changes:

screen shot 2018-09-06 at 11 44 17 am

screen shot 2018-09-06 at 11 44 29 am

@samvera/hyrax-code-reviewers

@no-reply
Copy link
Contributor

no-reply commented Sep 7, 2018

👏 This is a great contribution!

@@ -1,3 +1,3 @@
<div data-behavior="batch-add-button">
<%= check_box_tag "batch_document_ids[]", document.id, true, class:"batch_document_selector", id: "batch_document_#{document.id}" %>
</div>
<%= check_box_tag "batch_document_ids[]", document.id, false, class:"batch_document_selector", id: "batch_document_#{document.id}" %>
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you comment about how this change is related? I don't follow at a glance (I guess it has something to do with fallout from the javascript changes?) and want to make sure someone picking up the code later could understand.

Copy link
Contributor Author

@hweng hweng Sep 10, 2018

Choose a reason for hiding this comment

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

@no-reply Thanks for reviewing! I tested that without explicitly defining, the checked attribute for check_box_tag( with class:"batch_document_selector") default to true. The test was done in the master branch which is before this javascript change.
So I think it has to explicitly define as false to make it unchecked when page loaded. Is this the behavior that we wanted?

@@ -18,11 +18,11 @@
<% else %>

<%= form_tag embargoes_path, method: :patch do %>
<%= submit_tag t('.deactivate_selected'), class: 'btn btn-primary' %>
<%= submit_tag t('.deactivate_selected'), class: 'btn btn-primary' %>
Copy link
Contributor

Choose a reason for hiding this comment

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

If you have a chance, let's get rid of the trailing whitespace on this line.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sure

@no-reply no-reply added this to the 2.x series milestone Sep 7, 2018
no-reply
no-reply previously approved these changes Sep 10, 2018
Copy link
Contributor

@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.

👍

Copy link
Contributor

@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.

👍

@no-reply no-reply merged commit 57e477b into master Sep 11, 2018
@no-reply no-reply deleted the select_all branch September 11, 2018 17:01
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.

3 participants