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

integrated camera feature #385

Merged
merged 1 commit into from
Jun 2, 2024
Merged

integrated camera feature #385

merged 1 commit into from
Jun 2, 2024

Conversation

srsingh04
Copy link
Contributor

Integrated a camera feature through which now the user can take a take a picture/selfie and upload it as their profile picture.
Changing back the ProfileCreationviewModel back to an old version as well to fix a bug in the UI update.

Copy link
Contributor

@alejandrocalles alejandrocalles left a comment

Choose a reason for hiding this comment

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

Everything seems to work really well! Thank you :) Good job and really good addition.

A small UI comment: I believe the 'edit' overlay on the profile picture (in the profile screen) shouldn't be there. The user would expect said button to trigger the camera, but there's already a camera button that does that. That would also allow the user to see their profile picture more clearly.

Also a small note on logs. I believe they should be removed after debugging, or at least keep those that are related to relatively important errors (e.g. the user denied permissions, etc), and not those that are related to simple UI updates, for instance, like 'Last name field'. For those that are important, you could use priorities other than d (debugging), such as w (warning) or e (error).

@srsingh04
Copy link
Contributor Author

srsingh04 commented Jun 2, 2024

I believe the 'edit' overlay on the profile picture (in the profile screen) shouldn't be there. The user would expect said button to trigger the camera, but there's already a camera button that does that.

This part of the feature has been implemented by @unglazedstamp and I think how it differs from the camera is for being solely there as a functionality to select a picture from the personal gallery of the user. Nonetheless, I personally agree with your idea and think it's actually better to display the profile picture in large and clear and maybe put the edit icon on the circumference of the profile picture circle. This can be an interesting task for a future PR!

@alejandrocalles
Copy link
Contributor

alejandrocalles commented Jun 2, 2024

I understand. Is it possible to remove the camera button and connect the camera launcher to the edit button instead? It's just weird to have a non-functional button in production code.

@unglazedstamp unglazedstamp self-requested a review June 2, 2024 15:12
Copy link

@unglazedstamp unglazedstamp left a comment

Choose a reason for hiding this comment

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

Nice feature. I found some improvements in the code. Also I agree that it doesn't make sense to have an icon button to take a picture and an edit button on the picture to get a picture from the gallery. It is preferable to either have a dialog composable to do the choice once the user press on the edit button on the picture or have two icon buttons below the picture (one for each option).

}
) {
Text(stringResource(R.string.edit_event_screen_confirm))
}
}
}
}

private fun handleCapturedImage(bitmap: Bitmap, setPicture: (Bitmap) -> Unit) {
Copy link

@unglazedstamp unglazedstamp Jun 2, 2024

Choose a reason for hiding this comment

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

This functions should have comments

setPicture(bitmap)
}

private fun dispatchTakePictureIntent(cameraLauncher: ActivityResultLauncher<Intent>) {
Copy link

@unglazedstamp unglazedstamp Jun 2, 2024

Choose a reason for hiding this comment

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

This function should have comments

@srsingh04
Copy link
Contributor Author

srsingh04 commented Jun 2, 2024

It's just weird to have a non-functional button in production code.

The button is not completely non functional as such...it allows the user to choose amongst multiple images which they have already stored in the past, along with the ones they have clicked using the camera. The feature may come out as a bit confusing to the users in the beginning but unfortunately due to time constraints, I prefer not to touch the code furthermore in the PR. I agree that a dialog box linking it with the camera and a choice of choosing from the gallery would definitely make wayyyy more sense.
Thank you for your feedback regardless!

@alejandrocalles alejandrocalles enabled auto-merge (rebase) June 2, 2024 15:58
@alejandrocalles alejandrocalles dismissed their stale review June 2, 2024 19:45

Time constraints.

@srsingh04 srsingh04 self-assigned this Jun 2, 2024
@srsingh04 srsingh04 added this to the Milestone 4 milestone Jun 2, 2024
@Greenade Greenade self-requested a review June 2, 2024 19:50
@srsingh04 srsingh04 linked an issue Jun 2, 2024 that may be closed by this pull request
Copy link
Contributor

@Greenade Greenade left a comment

Choose a reason for hiding this comment

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

LGTM, could have been better as said by the others but due to time constraints we will merge it like this. This is still a very good addition to the app

camera working with the icon

camera feature integrated

addding comments to all my files

comments added
@violoncelloCH
Copy link
Member

violoncelloCH commented Jun 2, 2024

I squashed all the commits to remove all the "debug" and similar commit messages.
Tested and works on my phone, so should be ready to merge.

Copy link

sonarqubecloud bot commented Jun 2, 2024

Quality Gate Failed Quality Gate failed

Failed conditions
57.5% Line Coverage on New Code (required ≥ 80.0%)

See analysis details on SonarCloud

@alejandrocalles alejandrocalles merged commit c25828b into main Jun 2, 2024
1 of 2 checks passed
@alejandrocalles alejandrocalles deleted the feature/camera branch June 2, 2024 20:32
@srsingh04 srsingh04 linked an issue Jun 2, 2024 that may be closed by this pull request
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants