Feature: maximise event picture when clicking on it #304
Conversation
|
From 7.14% to 87.1%, bravo ! 🚀 |
|
Road to 1000% coverage 🚀 😎 |
Romainhir
left a comment
Romainhir
left a comment
There was a problem hiding this comment.
Nice job, a few small details to clarify and I'll approve it
| EventUserPicture("34", "http:image.com", User.emptyFirestoreReferenceElement(), 3)) | ||
| EventUserPicture( | ||
| "12", | ||
| "https://i1.sndcdn.com/artworks-000055240636-kowvdx-t500x500.jpg", |
There was a problem hiding this comment.
What happend if the URL is not available (like a 404 error) ? If it does not make the tests crash, you can leave it as this, but otherwise consider downloading the image or use one already downloaded
There was a problem hiding this comment.
Yes you are right, it made the test fail. I found a way to get the local Uri of a drawable instead.
| matcher = | ||
| hasTestTag(EventCreationTestTags.LOCATION_SUGGESTION_ITEM_LATITUDE + EVENT_LATITUDE), | ||
| timeoutMillis = 10000) | ||
| timeoutMillis = 15000) |
There was a problem hiding this comment.
Waiting 15 seconds before timeout is a lot. Confirm me you really need to wait that long, otherwise, reduce the sleeping time
There was a problem hiding this comment.
Nice spot, I had a problem with the test and wanted to see if it was due to insufficient delay, but I forgot to reset the correct timeout
| if (pagerState.currentPage > 0) { | ||
| scope.launch { pagerState.animateScrollToPage(pagerState.currentPage - 1) } | ||
| } else {} | ||
| } else {} |
There was a problem hiding this comment.
Empty else statement. Consider handle these cases properly
There was a problem hiding this comment.
I did this because android studio was giving me the warning "'if' must have both main and 'else' branches if used as an expression" when I didn't put any else statement. But found that casting onClickArrow to (Boolean) -> Unit achieves the same and is cleaner than empty else statements :)
| if (isRight && pagerState.currentPage < eventPictures.size - 1) { | ||
| scope.launch { pagerState.animateScrollToPage(pagerState.currentPage + 1) } | ||
| } else if (!isRight) { | ||
| if (pagerState.currentPage > 0) { |
There was a problem hiding this comment.
Why did you make two if statements ? Why did you not put it in a single one like you did just before (with an AND operator) ?
There was a problem hiding this comment.
This wasn't intentional, I corrected it
Test failed when image was not found
|
The goal of this PR is to make the user pictures gallery more user-friendly and interactive by allowing user to click on a picture in order to view it in full screen. Additionnaly, the user can navigate between the full-screen pictures by scrolling horizontally or clicking on the right/left arrow buttons.