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 section to trial details page #5337

Open
wants to merge 15 commits into
base: master
Choose a base branch
from
Open

Conversation

bm743
Copy link
Contributor

@bm743 bm743 commented Feb 20, 2025

Description

Adds an image section to the trial details page that displays all of the images from every plot associated with the trial

Closes #5276

Checklist

  • Refactoring only
  • Documentation only
  • Fixture update only
  • Bug fix
    • The relevant issue has been closed.
    • Further work is required.
  • New feature
    • Relevant tests have been created and run.
    • Data was added to the fixture
      • Data was added via a patch in /t/data/fixture/patches/.
    • User-Facing Change
      • The user manual in /docs has been updated.
    • Any new Perl has been documented using perldoc.
    • Any new JavaScript has been documented using JSDoc.
    • Any new legacy JavaScript has been moved from /js to /js/source/legacy.

@lukasmueller
Copy link
Member

add accession name to table, make images a bit larger, allow sorting by columns

@ryan-preble
Copy link
Contributor

I was not able to get this to run on cassava-devel. Tried two different trials and both hung on opening the trial images tab. Eventually got a javascript warning:
image
Also got a bunch of errors in the javascript console, but I am not sure they are related to this branch at all.

For reference, the two trials I tried to view (and I do not know that they have images at all): trial ID 541 and trial ID 1228

@lukasmueller lukasmueller self-requested a review March 3, 2025 16:06
Copy link
Member

@lukasmueller lukasmueller left a comment

Choose a reason for hiding this comment

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

Hi Ben, this looks great. A nice addition would be to add a button or link to an image upload facility. What do you think?

@bm743
Copy link
Contributor Author

bm743 commented Mar 4, 2025

@lukasmueller That sounds like a good idea, I will add that!

@@ -111,6 +111,24 @@ sub image_search :Path('/ajax/search/images') Args(0) {
my $image_page = "/image/view/$image_id";
my $colorbox = qq|<a href="$image_img" title="<a href=$image_page>Go to image page ($image_name)</a>" class="image_search_group" rel="gallery-figures"><img src="$small_image" width="40" height="30" border="0" alt="$image_description" /></a>|;
Copy link
Member

Choose a reason for hiding this comment

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

in the img src tag, do not specify both width and height because that distorts images. I would just specify width, because that's important for the table, and the height is whatever it is

Copy link
Contributor

@afpowell afpowell left a comment

Choose a reason for hiding this comment

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

👍

@@ -12,6 +12,10 @@ $stockref => undef

<& /util/import_javascript.mas, classes => [ 'jquery', 'thickbox', 'jquery.dataTables' ] &>

<script src="https://cdnjs.cloudflare.com/ajax/libs/jszip/3.7.1/jszip.min.js"></script>
Copy link
Member

Choose a reason for hiding this comment

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

This could also be done in the backend? If javascript, install it with npm?

Copy link
Member

@lukasmueller lukasmueller left a comment

Choose a reason for hiding this comment

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

Just two small comments. Let me know what you think

@@ -40,7 +40,7 @@ sub image_search :Path('/ajax/search/images') Args(0) {

my @stock_name_list;
if (exists($params->{image_stock_uniquename}) && $params->{image_stock_uniquename}) {
push @stock_name_list, $params->{image_stock_uniquename};
@stock_name_list = split /,/, $params->{image_stock_uniquename};
Copy link
Member

Choose a reason for hiding this comment

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

Will this be compatible with previous usage? When a url parameter is repeated, it will appear as a listref here. So maybe check for it being a listref and handle that case, and then handle the case of the comma separated string?


<script>

<%perl>
Copy link
Member

Choose a reason for hiding this comment

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

The authentication should not be needed, as the user is either unauthenticated and can't see this page, or they are already authenticataed and this shouldn't be necessary?

@bm743
Copy link
Contributor Author

bm743 commented Mar 22, 2025

Requires solgenomics/cxgn-corelibs#44 to be merged to pass tests

@lukasmueller
Copy link
Member

add a note to meatadata that something was deleted?

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.

Add a section to the trial detail page that contains all the images taken on the trial
4 participants