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

Display images and fix CI #126

Merged
merged 4 commits into from
Aug 23, 2023
Merged

Display images and fix CI #126

merged 4 commits into from
Aug 23, 2023

Conversation

leefaisonr
Copy link
Contributor

@leefaisonr leefaisonr commented Aug 18, 2023

Co-authored-by: Bess Sadler bess@users.noreply.github.com
Co-authored-by: Christina Chortaria christinach@users.noreply.github.com
Co-authored-by: Jane Sandberg sandbergja@users.noreply.github.com

Ref #19

leefaisonr and others added 3 commits August 18, 2023 17:07
 Co-authored-by: Bess Sadler <bess@users.noreply.github.com>
 Co-authored-by: Christina Chortaria <christinach@users.noreply.github.com>
 Co-authored-by: Jane Sandberg <sandbergja@users.noreply.github.com>
Copy link
Member

@hackartisan hackartisan left a comment

Choose a reason for hiding this comment

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

This looks great, a couple of small things and we should improve the system spec.

end

describe 'SubGuide page with image display' do
it 'shows all images for the SubGuide path' do
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure this is true. The test doesn't check for image tags, for example. It appears to check for some kind of title or heading.

I understand that we don't want to run the card image loading service in tests, because it hits aws. Instead, to test the show page functionality, we could create a card image object with a false value and test that the image tag is produced on the page.

@@ -1,3 +1,7 @@
<h1>SubGuideCards#show</h1>
<p>Find me in app/views/sub_guide_cards/show.html.erb</p>
<%= @sub_guide_card.heading %>
<%= @sub_guide_card.path %>
<% @card_images.each do |image| %>
<%= image_tag(image.iiif_url) %>
Copy link
Member

Choose a reason for hiding this comment

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

Please indent

@@ -1,3 +1,7 @@
<h1>SubGuideCards#show</h1>
<p>Find me in app/views/sub_guide_cards/show.html.erb</p>
<%= @sub_guide_card.heading %>
<%= @sub_guide_card.path %>
Copy link
Member

Choose a reason for hiding this comment

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

Do we want to print the sub guide card path on the show page? Maybe it was printed for debugging and we can remove it?

Co-authored-by: Anna Headley <hackartisan@users.noreply.github.com>
Copy link
Member

@hackartisan hackartisan left a comment

Choose a reason for hiding this comment

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

🎉

@hackartisan hackartisan merged commit c370fff into main Aug 23, 2023
@hackartisan hackartisan deleted the display-subguide-images branch August 23, 2023 14:36
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.

2 participants