Add selective download and print for collections #965
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Implement feature to allow users to choose between downloading/printing the entire collection or only selected items. Changes: - Modified CollectionsController download and print actions to accept manifestation_ids[] parameter for selective operations - Created print modal similar to download modal with item selection - Updated download modal to include toggle between full/partial collection - Added manifestation checkboxes list with select-all functionality - Added I18n strings for Hebrew and English - Added helper method print_url_by_entity - Updated collection_top partial to trigger print modal instead of direct print - Added system spec for testing selective download/print functionality Fixes #by-sxo Co-Authored-By: Claude Sonnet 4.5 <[email protected]>
- Make modals larger (modal-lg) to accommodate item lists - Add scrollable area for manifestation lists (max-height: 400px) - Apply fixes to both download and print modals Fixes issue where item list was clipped when modal didn't resize. Co-Authored-By: Claude Sonnet 4.5 <[email protected]>
- Set min-height: 60vh on .by-card-content-v02 to make modal much larger - Increase scrollable list height to 50vh (from 400px) for better visibility - Add overflow-x: hidden to prevent horizontal scrolling - Apply to both download and print modals This ensures the modal is large from the start and can accommodate many items without clipping. Co-Authored-By: Claude Sonnet 4.5 <[email protected]>
- Move .bottom-button-area outside of .by-card-content-v02 - Make it a sibling element instead of child - Use flex: 0 0 auto to prevent button area from growing/shrinking - Use margin-top: auto to push buttons to bottom - Make content area scrollable with flex: 1 1 auto This ensures download/print and cancel buttons stay at the bottom of the modal even when the item list is displayed. Co-Authored-By: Claude Sonnet 4.5 <[email protected]>
Accidentally removed max-height and overflow styles when restructuring button positioning. Restore the inline styles to make lists scrollable: - max-height: 40vh (reduced from 50vh to account for button area) - overflow-y: auto - overflow-x: hidden Without these, the lists were not visible/scrollable. Co-Authored-By: Claude Sonnet 4.5 <[email protected]>
Override .flex-popup height: auto with explicit height: 75vh to ensure modal is large enough to display item lists properly. Applied to both download and print modals. Co-Authored-By: Claude Sonnet 4.5 <[email protected]>
The .flex-popup was correctly 75vh, but child .by-popup-v02 was only 273px instead of filling the parent. Added inline styles to .by-popup-v02: - height: 100% - fill parent - display: flex; flex-direction: column - proper flex layout Verified with Capybara: - .by-popup-v02: 946px (was 273px) - .by-card-content-v02: 813px (was 140px) Co-Authored-By: Claude Sonnet 4.5 <[email protected]>
The .field-v02 class has height: 28px which was constraining the .texts-list container, causing the manifestation list to overflow and overlap the format radio buttons below it. Added inline style height: auto to .field-v02.texts-list to allow the container to expand and properly push content below. Verified with Capybara: - Manifestation area height: 570px (was 51px) - Radio buttons moved down 569px (was only 51px) - No overlap detected Applied to both download and print modals. Co-Authored-By: Claude Sonnet 4.5 <[email protected]>
1. Changed .by-card-content-v02 overflow-y from 'auto' to 'hidden' - Prevents overall modal scrolling - Only the item list itself scrolls (has max-height: 40vh) 2. Changed .field-v02.texts-list background to gray - background-color: #eeeced (matches by-card-v02) - border: none (removes white field border) Applied to both download and print modals. Co-Authored-By: Claude Sonnet 4.5 <[email protected]>
1. Added slightly lighter gray background to list items - ol background-color: #f5f3f4 (vs #eeeced for container) - Provides subtle visual distinction 2. Added horizontal rule in download modal - <hr> tag between manifestation list and file formats - Provides clear separation between sections Applied to both download and print modals. Co-Authored-By: Claude Sonnet 4.5 <[email protected]>
- Moved HR to correct position: between manifestation area and format section (reduced indentation to match download_instructions) - Added ID #list-separator for JavaScript control - Updated JavaScript to show/hide HR along with manifestation list - HR starts hidden and only appears when user selects "partial" mode This ensures the separator appears between the item list and file formats, not between the count and items. Co-Authored-By: Claude Sonnet 4.5 <[email protected]>
The _reboot.scss sets hr height:0, making it invisible. Added inline styles to override: - height: 1px - border: none - border-top: 1px solid #d9cdd5 (matching site border color) - margin: 15px 0 (spacing above and below) Co-Authored-By: Claude Sonnet 4.5 <[email protected]>
Changed border color from #d9cdd5 (light) to #999 (medium gray) for better visibility and clearer section separation. Co-Authored-By: Claude Sonnet 4.5 <[email protected]>
Changed overflow-y from 'hidden' back to 'auto' on .by-card-content-v02. The issue: With overflow-y: hidden, when content exceeded container height (1075px content in 1004px space), radio buttons at the bottom were clipped and invisible. The solution: Allow content area to scroll when needed. The action buttons stay visible at bottom because they're in a separate flex container (.bottom-button-area) outside the scrollable content. This provides: - Scrollable content when there's overflow (can see all radios) - Action buttons always visible at bottom (fixed position) - Item list scrolls internally (max-height: 40vh) Applied to both download and print modals. Co-Authored-By: Claude Sonnet 4.5 <[email protected]>
Changes: 1. Removed max-height constraint from item lists - No separate scroll for list, everything scrolls together - Smoother single scrollbar experience 2. Increased modal height from 75vh to 90vh - Takes up more viewport space - More content visible at once 3. Positioned modal closer to top - Added margin-top: 2vh to .modal-dialog - Modal appears near top of screen instead of centered Applied to both download and print modals. Co-Authored-By: Claude Sonnet 4.5 <[email protected]>
The button text was unconditionally set to "שמירת האסופה בפורמט הנבחר" (download_full_collection) even for manifestation pages. This broke existing tests that expected the button to say "הורדה" (download). Changes: - Made button text conditional based on entity type - For Manifestations: Use t(:download) / t(:print) - For Collections: Use t(:download_full_collection) / t(:print_full_collection) - Applied to both download and print modals Fixes spec/system/download_modal_spec.rb failures. Co-Authored-By: Claude Sonnet 4.5 <[email protected]>
Co-Authored-By: Claude Sonnet 4.5 <[email protected]>
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| #downloadDlg.modal{'aria-hidden' => 'true', role: 'dialog', tabindex: '-1'} | ||
| = render partial: 'shared/download', locals: {entity: @collection} | ||
|
|
||
| #printDlg.modal{'aria-hidden' => 'true', role: 'dialog', tabindex: '-1'} |
There was a problem hiding this comment.
Classes should be listed before IDs (.modal should precede #printDlg)
| #downloadDlg.modal{'aria-hidden' => 'true', role: 'dialog', tabindex: '-1'} | ||
| = render partial: 'shared/download', locals: {entity: @collection} | ||
|
|
||
| #printDlg.modal{'aria-hidden' => 'true', role: 'dialog', tabindex: '-1'} |
There was a problem hiding this comment.
Hash attribute should start with one space after the opening brace
| #downloadDlg.modal{'aria-hidden' => 'true', role: 'dialog', tabindex: '-1'} | ||
| = render partial: 'shared/download', locals: {entity: @collection} | ||
|
|
||
| #printDlg.modal{'aria-hidden' => 'true', role: 'dialog', tabindex: '-1'} |
There was a problem hiding this comment.
Hash attribute should end with one space before the closing brace or be on its own line
| = render partial: 'shared/download', locals: {entity: @collection} | ||
|
|
||
| #printDlg.modal{'aria-hidden' => 'true', role: 'dialog', tabindex: '-1'} | ||
| = render partial: 'shared/print_modal', locals: {entity: @collection} |
There was a problem hiding this comment.
Layout/SpaceInsideHashLiteralBraces: Space inside { missing.
| = render partial: 'shared/download', locals: {entity: @collection} | ||
|
|
||
| #printDlg.modal{'aria-hidden' => 'true', role: 'dialog', tabindex: '-1'} | ||
| = render partial: 'shared/print_modal', locals: {entity: @collection} |
There was a problem hiding this comment.
Layout/SpaceInsideHashLiteralBraces: Space inside } missing.
| .headline-3-v02= t(:included_items) | ||
| .mainlist.multiselect#texts-to-save | ||
| %ol{style: 'background-color: #f5f3f4;'} | ||
| - entity.flatten_items.select { |ci| ci.item_type == 'Manifestation' && ci.item.present? }.each do |ci| |
| %ol{style: 'background-color: #f5f3f4;'} | ||
| - entity.flatten_items.select { |ci| ci.item_type == 'Manifestation' && ci.item.present? }.each do |ci| | ||
| %li | ||
| %input.multiselect_checkbox{type: 'checkbox', name: 'manifestation_ids[]', value: ci.item_id} |
| %ol{style: 'background-color: #f5f3f4;'} | ||
| - entity.flatten_items.select { |ci| ci.item_type == 'Manifestation' && ci.item.present? }.each do |ci| | ||
| %li | ||
| %input.multiselect_checkbox{type: 'checkbox', name: 'manifestation_ids[]', value: ci.item_id} |
There was a problem hiding this comment.
Hash attribute should start with one space after the opening brace
| %ol{style: 'background-color: #f5f3f4;'} | ||
| - entity.flatten_items.select { |ci| ci.item_type == 'Manifestation' && ci.item.present? }.each do |ci| | ||
| %li | ||
| %input.multiselect_checkbox{type: 'checkbox', name: 'manifestation_ids[]', value: ci.item_id} |
There was a problem hiding this comment.
Hash attribute should end with one space before the closing brace or be on its own line
| - entity.flatten_items.select { |ci| ci.item_type == 'Manifestation' && ci.item.present? }.each do |ci| | ||
| %li | ||
| %input.multiselect_checkbox{type: 'checkbox', name: 'manifestation_ids[]', value: ci.item_id} | ||
| - author_names = ci.involved_authorities.select { |ia| ia.role == 'author' }.map { |ia| ia.authority.name }.join(', ') |
| - text = author_names.present? ? "#{ci.title} / #{author_names}" : ci.title | ||
| = text | ||
|
|
||
| %hr#list-separator{style: 'display: none; height: 1px; border: none; border-top: 1px solid #999; margin: 15px 0;'} |
| - entity.flatten_items.select { |ci| ci.item_type == 'Manifestation' && ci.item.present? }.each do |ci| | ||
| %li | ||
| %input.print-multiselect_checkbox{type: 'checkbox', name: 'manifestation_ids[]', value: ci.item_id} | ||
| - author_names = ci.involved_authorities.select { |ia| ia.role == 'author' }.map { |ia| ia.authority.name }.join(', ') |
| %span.right-side-icon - | ||
| %span{:style => "font-weight: bold"}= t(:cancel) | ||
| - button_text = entity.is_a?(Collection) ? t(:print_full_collection) : t(:print) | ||
| = submit_tag button_text, {class: 'pointer by-button-v02 link-to-all-v02 print_submitter', id: 'print_do_print'} |
| %span.right-side-icon - | ||
| %span{:style => "font-weight: bold"}= t(:cancel) | ||
| - button_text = entity.is_a?(Collection) ? t(:print_full_collection) : t(:print) | ||
| = submit_tag button_text, {class: 'pointer by-button-v02 link-to-all-v02 print_submitter', id: 'print_do_print'} |
There was a problem hiding this comment.
Layout/SpaceInsideHashLiteralBraces: Space inside { missing.
| %span.right-side-icon - | ||
| %span{:style => "font-weight: bold"}= t(:cancel) | ||
| - button_text = entity.is_a?(Collection) ? t(:print_full_collection) : t(:print) | ||
| = submit_tag button_text, {class: 'pointer by-button-v02 link-to-all-v02 print_submitter', id: 'print_do_print'} |
There was a problem hiding this comment.
Layout/SpaceInsideHashLiteralBraces: Space inside } missing.
| .flex-popup{style: 'height: 90vh;'} | ||
| .modal-header | ||
| .popup-top | ||
| %button.btn-small-outline-v02{'data-dismiss'=>'modal'} |
There was a problem hiding this comment.
Layout/SpaceAroundOperators: Surrounding space missing for operator =>.
| .flex-popup{style: 'height: 90vh;'} | ||
| .modal-header | ||
| .popup-top | ||
| %button.btn-small-outline-v02{'data-dismiss'=>'modal'} |
There was a problem hiding this comment.
Hash attribute should start with one space after the opening brace
| .flex-popup{style: 'height: 90vh;'} | ||
| .modal-header | ||
| .popup-top | ||
| %button.btn-small-outline-v02{'data-dismiss'=>'modal'} |
There was a problem hiding this comment.
Hash attribute should end with one space before the closing brace or be on its own line
| .btn-text-v02 | ||
| %span.right-arrow 2 | ||
| %span= t(:back) | ||
| .headline-2-v02{:style => "text-align: center;margin: 0; padding-top: 3px;"}= t(:print) |
There was a problem hiding this comment.
Style/HashSyntax: Use the new Ruby 1.9 hash syntax.
| .btn-text-v02 | ||
| %span.right-arrow 2 | ||
| %span= t(:back) | ||
| .headline-2-v02{:style => "text-align: center;margin: 0; padding-top: 3px;"}= t(:print) |
There was a problem hiding this comment.
Style/StringLiterals: Prefer single-quoted strings when you don't need string interpolation or special symbols.
| .btn-text-v02 | ||
| %span.right-arrow 2 | ||
| %span= t(:back) | ||
| .headline-2-v02{:style => "text-align: center;margin: 0; padding-top: 3px;"}= t(:print) |
There was a problem hiding this comment.
Hash attribute should start with one space after the opening brace
| .btn-text-v02 | ||
| %span.right-arrow 2 | ||
| %span= t(:back) | ||
| .headline-2-v02{:style => "text-align: center;margin: 0; padding-top: 3px;"}= t(:print) |
There was a problem hiding this comment.
Hash attribute should end with one space before the closing brace or be on its own line
| .by-popup-v02{style: 'height: 100%; display: flex; flex-direction: column;'} | ||
| .by-card-header-v02.desktop-only | ||
| %span.headline-1-v02= t(:print) | ||
| %a.popup-x-v02.linkcolor.pointer{'data-dismiss'=>'modal'} - |
There was a problem hiding this comment.
Layout/SpaceAroundOperators: Surrounding space missing for operator =>.
| .by-popup-v02{style: 'height: 100%; display: flex; flex-direction: column;'} | ||
| .by-card-header-v02.desktop-only | ||
| %span.headline-1-v02= t(:print) | ||
| %a.popup-x-v02.linkcolor.pointer{'data-dismiss'=>'modal'} - |
There was a problem hiding this comment.
Hash attribute should start with one space after the opening brace
| .by-popup-v02{style: 'height: 100%; display: flex; flex-direction: column;'} | ||
| .by-card-header-v02.desktop-only | ||
| %span.headline-1-v02= t(:print) | ||
| %a.popup-x-v02.linkcolor.pointer{'data-dismiss'=>'modal'} - |
There was a problem hiding this comment.
Hash attribute should end with one space before the closing brace or be on its own line
|
|
||
| = hidden_field_tag 'print_scope', 'full', id: 'print_scope' | ||
| .bottom-button-area.left-buttons{style: 'flex: 0 0 auto; margin-top: auto;'} | ||
| .desktop-only.linkcolor.pointer{'data-dismiss'=>'modal'} |
There was a problem hiding this comment.
Layout/SpaceAroundOperators: Surrounding space missing for operator =>.
|
|
||
| = hidden_field_tag 'print_scope', 'full', id: 'print_scope' | ||
| .bottom-button-area.left-buttons{style: 'flex: 0 0 auto; margin-top: auto;'} | ||
| .desktop-only.linkcolor.pointer{'data-dismiss'=>'modal'} |
There was a problem hiding this comment.
Hash attribute should start with one space after the opening brace
|
|
||
| = hidden_field_tag 'print_scope', 'full', id: 'print_scope' | ||
| .bottom-button-area.left-buttons{style: 'flex: 0 0 auto; margin-top: auto;'} | ||
| .desktop-only.linkcolor.pointer{'data-dismiss'=>'modal'} |
There was a problem hiding this comment.
Hash attribute should end with one space before the closing brace or be on its own line
| .bottom-button-area.left-buttons{style: 'flex: 0 0 auto; margin-top: auto;'} | ||
| .desktop-only.linkcolor.pointer{'data-dismiss'=>'modal'} | ||
| %span.right-side-icon - | ||
| %span{:style => "font-weight: bold"}= t(:cancel) |
There was a problem hiding this comment.
Style/HashSyntax: Use the new Ruby 1.9 hash syntax.
| .bottom-button-area.left-buttons{style: 'flex: 0 0 auto; margin-top: auto;'} | ||
| .desktop-only.linkcolor.pointer{'data-dismiss'=>'modal'} | ||
| %span.right-side-icon - | ||
| %span{:style => "font-weight: bold"}= t(:cancel) |
There was a problem hiding this comment.
Style/StringLiterals: Prefer single-quoted strings when you don't need string interpolation or special symbols.
| .bottom-button-area.left-buttons{style: 'flex: 0 0 auto; margin-top: auto;'} | ||
| .desktop-only.linkcolor.pointer{'data-dismiss'=>'modal'} | ||
| %span.right-side-icon - | ||
| %span{:style => "font-weight: bold"}= t(:cancel) |
There was a problem hiding this comment.
Hash attribute should start with one space after the opening brace
| .bottom-button-area.left-buttons{style: 'flex: 0 0 auto; margin-top: auto;'} | ||
| .desktop-only.linkcolor.pointer{'data-dismiss'=>'modal'} | ||
| %span.right-side-icon - | ||
| %span{:style => "font-weight: bold"}= t(:cancel) |
There was a problem hiding this comment.
Hash attribute should end with one space before the closing brace or be on its own line
| @@ -0,0 +1,109 @@ | |||
| .modal-dialog.modal-lg{style: 'margin-top: 2vh;'} | |||
There was a problem hiding this comment.
Hash attribute should start with one space after the opening brace
| @@ -0,0 +1,109 @@ | |||
| .modal-dialog.modal-lg{style: 'margin-top: 2vh;'} | |||
There was a problem hiding this comment.
Hash attribute should end with one space before the closing brace or be on its own line
| - formurl = print_url_by_entity(entity) | ||
| - formid = "print_form_#{entity.class.name.downcase}_#{entity.id}" | ||
| = form_tag(formurl, method: :get, id: formid, target: '_blank') do | ||
| .flex-popup{style: 'height: 90vh;'} |
There was a problem hiding this comment.
Hash attribute should start with one space after the opening brace
| - formurl = print_url_by_entity(entity) | ||
| - formid = "print_form_#{entity.class.name.downcase}_#{entity.id}" | ||
| = form_tag(formurl, method: :get, id: formid, target: '_blank') do | ||
| .flex-popup{style: 'height: 90vh;'} |
There was a problem hiding this comment.
Hash attribute should end with one space before the closing brace or be on its own line
| .headline-2-v02{:style => "text-align: center;margin: 0; padding-top: 3px;"}= t(:print) | ||
| .button-placeholder | ||
| .modal-body | ||
| .by-popup-v02{style: 'height: 100%; display: flex; flex-direction: column;'} |
There was a problem hiding this comment.
Hash attribute should start with one space after the opening brace
| .headline-2-v02{:style => "text-align: center;margin: 0; padding-top: 3px;"}= t(:print) | ||
| .button-placeholder | ||
| .modal-body | ||
| .by-popup-v02{style: 'height: 100%; display: flex; flex-direction: column;'} |
There was a problem hiding this comment.
Hash attribute should end with one space before the closing brace or be on its own line
| .by-card-header-v02.desktop-only | ||
| %span.headline-1-v02= t(:print) | ||
| %a.popup-x-v02.linkcolor.pointer{'data-dismiss'=>'modal'} - | ||
| .by-card-content-v02{style: 'overflow-y: auto; flex: 1 1 auto;'} |
There was a problem hiding this comment.
Hash attribute should start with one space after the opening brace
abartov
deleted the
feature/by-sxo-selective-collection-download-print
branch
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
Implements selective download and print functionality for collections, allowing users to choose between downloading/printing the entire collection or only selected items.
Changes
Backend
CollectionsController#downloadandCollectionsController#printto acceptmanifestation_ids[]parameter for selective operationsprint_url_by_entityto generate print URLsFrontend
_print_modal.html.haml) similar to download modal with item selectionLocalization
download_or_print_choose_scopefull_collectionselected_itemsitems_to_saveitems_to_printincluded_itemsformat_to_savedownload_full_collectionprint_full_collectionTesting
collection_selective_download_print_spec.rb) testing:download_modal_spec.rbtests by making button text conditionalTest Results
All specs passing:
spec/system/collection_selective_download_print_spec.rb: 7 examples, 0 failuresspec/system/download_modal_spec.rb: 2 examples, 0 failuresFixes
Closes #by-sxo
🤖 Generated with Claude Code