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

Implement attachment viewer gallery in record sets #3363

Merged
merged 31 commits into from
Nov 1, 2023

Conversation

CarolineDenis
Copy link
Contributor

Fixes #2132

@CarolineDenis CarolineDenis requested review from maxpatiiuk and a team April 17, 2023 21:28
@maxpatiiuk
Copy link
Member

To test:
(besides the obvious things)
test editing attachments and make sure the edits are preserved when you click "save" - it's possible that we have a bug where attachments would only be saved by the "save" button on the form if you edited attachments for the record that is currently displayed on the form

@CarolineDenis CarolineDenis removed the request for review from a team April 25, 2023 14:04
@maxpatiiuk maxpatiiuk requested a review from a team April 27, 2023 14:44
Copy link
Contributor

@chanulee1 chanulee1 left a comment

Choose a reason for hiding this comment

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

Fetching more attachments results in obtrusive loading dialog. Also, the number of attachments displayed at the top of the viewer seems to count the number of attachments currently fetched, not total number of attachments. Expected?

Cm6o3fwtSF.mp4

https://coldfish-issue-2132.test.specifysystems.org/specify/view/collectionobject/162/?recordsetid=157

@chanulee1
Copy link
Contributor

Cannot edit attachments through attachment viewer gallery. Must navigate to the associated record and make edits there to see changes in attachment viewer gallery, and then reload to see changes in the record set.

LG3kFqbsXa.mp4

https://coldfish-issue-2132.test.specifysystems.org/specify/view/collectionobject/162/?recordsetid=157

Triggered by dab39e4 on branch refs/heads/issue-2132
Copy link
Contributor

@chanulee1 chanulee1 left a comment

Choose a reason for hiding this comment

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

Skeleton loader looks really nice. But fetching more records seems to fetch every remaining record. Also, the loading gifs appear right before records are finally loaded and noticed slight cutoff on the right side of the third column of attachments.

8vVq0dw0BZ.mp4

@CarolineDenis
Copy link
Contributor Author

@chanulee1: regarding your comment "noticed slight cutoff on the right side of the third column of attachments."
Do you think you might have resized your dialog when testing this? I tried to reproduce it on my end and also launched the branch you created on the test panel but didn't have a cutoff. See recording below.

Screen.Recording.2023-05-22.at.8.54.13.AM.mov

@chanulee1
Copy link
Contributor

chanulee1 commented May 22, 2023

I was on Chrome too... Strange

@maxpatiiuk
Copy link
Member

Yes, the cut-off was likely caused by manually resizing the dialog.
The dialog sizes are stored in the browser, thus changing the browser, or accessing the same specify user from a different computer would result in different behavior

@chanulee1
Copy link
Contributor

Hmm. The cutoff seems to happen for me no matter how it is resized. It looks like it is because of the thickness of the scrollbar? @CarolineDenis For some reason the scroll bar appears different in our videos. Not sure what determines this.

SepSL92k3R.mp4

@melton-jason
Copy link
Contributor

Hmm. The cutoff seems to happen for me no matter how it is resized. It looks like it is because of the thickness of the scrollbar? @CarolineDenis For some reason the scroll bar appears different in our videos. Not sure what determines this.

As far as I know, by default the scrollbars are determined by your Operating System.
I do not believe Specify has altered or changed the appearance of Scrollbars in any way, so it should be using the default Scrollbar for your OS.

@CarolineDenis CarolineDenis requested review from a team and removed request for grantfitzsimmons September 25, 2023 16:47
Copy link
Contributor

@pashiav pashiav left a comment

Choose a reason for hiding this comment

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

Looks good!

issue-2132.mov

@specifysoftware
Copy link

This pull request has been mentioned on Specify Community Forum. There might be relevant details there:

https://discourse.specifysoftware.org/t/specify-7-9-2-release-announcement/1452/1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Ability to see all attachments associated with a record set
8 participants