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

Add Image Captions on Thumbnail View #77

Open
wants to merge 8 commits into
base: improve-build
Choose a base branch
from

Conversation

warrenhbrown
Copy link

This pull request adds support for displaying a caption under the images in the thumbnail view. settings.json is extended with: displayImgCaption: when set to true, the fields (as configured in imageInfoFlds, see above) will render underneath preview views and the image detail view.

Copy link
Member

@grantfitzsimmons grantfitzsimmons left a comment

Choose a reason for hiding this comment

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

It looks like this PR includes the changes for #76 as well as the changes to add image captions in thumbnail views.

I really appreciate the way this was added (adding a new setting named displayImgCaption that can be toggled on or off) to make it simple and optional for users. Great work and thank you so much @warrenhbrown!

@@ -81,6 +81,9 @@
<script type="text/javascript" src="//maps.googleapis.com/maps/api/js?key=AIzaSyDYHJjsVAniNFUxSZnIO6pyVBxBqRt9gmw&sensor=false"></script>
<!--script type="text/javascript" src="//maps.google.com/maps/api/js?sensor=false"></script-->

<!-- Google Tag Manager -->
Copy link
Member

Choose a reason for hiding this comment

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

It looks like the same changes are made on #76

<!-- Google Tag Manager -->
<script type="module" src="app/view/gtm.js"></script>

Choose a reason for hiding this comment

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

If this was intentional, it may be worth noting that the Pull Requests are now 'out of sync': there is an extra commit in #76 which is not present in this PR: db2e7f2

Choose a reason for hiding this comment

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

Thanks for catching that @melton-jason — it looks like we had some extraneous merge commits from one of our other branches that was not intended to be a part of this feature branch. I have removed those commits and hopefully there are no longer any overlapping/out-of-sync concerns.

@grantfitzsimmons
Copy link
Member

@specify/dev-testing This code is live on their Fish Web Portal. When testing, we need to set up a new instance and verify that the settings.json can include a line that sets displayImgCaption to true

The current float-based layout for image thumbnails results in
irregular gaps due to the variability in image caption height.
Copy link

@jyoung-uf jyoung-uf left a comment

Choose a reason for hiding this comment

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

Here's an example of the layout issue we observed due to longer caption lengths that affect rendering height. The existing css float rule remains in place to support older browsers, if needed.

float-layout-issue

Commit w/ potential fix: d628eac

Choose a reason for hiding this comment

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

Apologies, but we realized that the previous change was not working properly for cases where more than two fields were configured for imageInfoFlds. This should address that.

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.

4 participants