feat: display photos in observations #344
Conversation
There was a problem hiding this comment.
I think this is a good change but three additional things are needed:
- This doesn't completely address Show photos in observation #229 because tapping a photo doesn't open a modal. See the screen recording in the issue for a reference. (I assume you'll need to fill in
handlePhotoPressinThumbnailScrollView.tsxto accomplish this.) ThumbnailScrollViewbug: when photos take a long time to load,ThumbnailScrollViewbug: when photos take a long time to load, there is no UI for this. A grayed-out square, or some other placeholder, should be shown.
All of these can be done in a followup PR.
|
Thanks for quick review, Evan 😃 You're right, this PR doesn't implement opening modal when you click a photo. I should have noted we're going to do this in another PR which also implements delete button in spoken modal. I'll address 3) (and 2) perhaps as I guess it's just typo) probably tomorrow morning for us. Also, is it okay for us to include modal changes along with observation editing (#65) or you'd like to see that in completely separate PR? |
There was a problem hiding this comment.
- perhaps as I guess it's just typo
Whoops, sorry for the typo.
When photos take a long time to load, ThumbnailScrollView will jump around unexpectedly. I reproduced this by adding 10 photos to an observation.
Again, this can probably be fixed in a followup PR.
Also, is it okay for us to include modal changes along with observation editing (#65) or you'd like to see that in completely separate PR?
I'd prefer a separate, smaller PR. Does that work for you?
| @@ -58,10 +64,9 @@ export const Thumbnail = ({photo, style, size, onPress}: ThumbnailProps) => { | |||
| ); | |||
| }; | |||
|
|
|||
| export const ThumbnailScrollView = () => { | |||
| export const ThumbnailScrollView = (props: {photos: (Photo | undefined)[]}) => { | |||
There was a problem hiding this comment.
It feels a little weird that we'd allow an array with undefined in it. What's the use case for that?
There was a problem hiding this comment.
it's so we can keep placeholders for still loading images
| const isCapturing = | ||
| (photo && 'capturing' in photo && (photo.capturing as boolean)) || | ||
| undefined; |
There was a problem hiding this comment.
Nitpick: I would expect isCapturing to be a boolean, not undefined | boolean. Can you make sure it's always a bool?
|
|
||
| const spacing = 10; | ||
| const minSize = 150; | ||
| const log = debug('Thumbnail'); | ||
|
|
||
| type ThumbnailProps = { | ||
| photo: Photo; | ||
| photo?: Photo; |
|
This PR will most likely need to be put on hold until #335 is approved as points made there are also applicable for this PR. |
|
Sounds good. Let me know when I should give this another look.
|
Part of #229