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

[2656] - refactor: Registration verification from XML to compose #2605

Conversation

akashmeruva9
Copy link
Collaborator

@akashmeruva9 akashmeruva9 commented Jun 12, 2024

Fixes #2584

VID-20240613-WA0004.mp4
  • [ x] Apply the AndroidStyle.xml style template to your code in Android Studio.

  • [x ] Run the unit tests with ./gradlew check to make sure you didn't break anything

  • [x ] If you have multiple commits please combine them into one commit by squashing them.

@akashmeruva9 akashmeruva9 changed the title Registration verification compose refactor: Registration verification compose Jun 12, 2024
@akashmeruva9 akashmeruva9 changed the title refactor: Registration verification compose refactor: Registration verification from XML to compose Jun 12, 2024
Copy link
Collaborator

@PratyushSingh07 PratyushSingh07 left a comment

Choose a reason for hiding this comment

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

i see that you have attached some screemshots and I was wondering if they are the same as the earlier XML designs. Also in the error state I think you should remove the Mifos Icon , it looks a bit odd . What do you have to say about this @AvneetSingh2001 ?

container: ViewGroup?,
savedInstanceState: Bundle?,
): View {
return mifosComposeView(requireContext()) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

set a view composition strategy below this line

Copy link
Collaborator

Choose a reason for hiding this comment

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

view Composition strategy is implemented in mifosComposeView already, so no need


/**
* Created by dilpreet on 31/7/17.
*/
@AndroidEntryPoint
class RegistrationVerificationFragment : BaseFragment() {
class RegistrationVerificationFragment_Old : BaseFragment() {
private var _binding: FragmentRegistrationVerificationBinding? = null
Copy link
Collaborator

Choose a reason for hiding this comment

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

remove this fragment if not needed

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Rajan sir asked to add the old fragments in the meeting, that's why i am keeping them

): View {
return mifosComposeView(requireContext()) {
RegistrationVerificationScreen(
registrationViewModel,
Copy link
Collaborator

Choose a reason for hiding this comment

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

no need of passing viewModel. we can omit this parameter as we already assigned hiltViewModel



@Composable
fun RegistrationVerificationContent(
Copy link
Collaborator

Choose a reason for hiding this comment

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

Use MFScaffold, and pass this content in the scaffoldContent of the scaffold

Comment on lines 109 to 111
containerColor = if (isSystemInDarkTheme()) Color(
0xFF9bb1e3
) else Color(0xFF325ca8)
Copy link
Collaborator

Choose a reason for hiding this comment

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

single line if

}

RegistrationUiState.Success -> {
context.startActivity(Intent(context, LoginActivity::class.java))
Copy link
Collaborator

Choose a reason for hiding this comment

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

dont start activity from here, do this in fragment, the callback should call this LoginActivity from fragment

Comment on lines 84 to 98
MifosOutlinedTextField(
value = requestID,
onValueChange = { requestID = it },
label = R.string.request_id,
supportingText = "",
keyboardType = KeyboardType.Number
)

MifosOutlinedTextField(
value = authenticationToken,
onValueChange = { authenticationToken = it },
label = R.string.authentication_token,
supportingText = "",
keyboardType = KeyboardType.Number
)
Copy link
Collaborator

Choose a reason for hiding this comment

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

do error handling as well for these textfields

Comment on lines 146 to 151
MifosProgressIndicator(
modifier = Modifier
.fillMaxSize()
.padding(10.dp)
.background(MaterialTheme.colorScheme.background.copy(alpha = 0.7f))
)
Copy link
Collaborator

Choose a reason for hiding this comment

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

User MifosProgressOverlayIndicator

Comment on lines 119 to 128
Image(
painterResource(R.drawable.mifos_logo),
contentDescription = "",
contentScale = ContentScale.Fit,
alignment = Alignment.Center,
modifier = Modifier
.padding(dimensionResource(id = R.dimen.Mifos_DesignSystem_Spacing_screenHorizontalMargin))
.height(dimensionResource(id = R.dimen.Mifos_DesignSystem_Size_LogoImageSize))
.fillMaxWidth()
)
Copy link
Collaborator

Choose a reason for hiding this comment

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

move this in content screen, show image only in the case of Initial state

},
Modifier
.fillMaxWidth()
.padding(start = 16.dp, end = 16.dp, top = 4.dp),
Copy link
Collaborator

Choose a reason for hiding this comment

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

give top padding 12

confirmButton = {
TextButton(onClick = { showConfirmationDialog = false
navigateBack.invoke() }) {
Text(text = "Yes")
Copy link
Collaborator

Choose a reason for hiding this comment

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

use stringResource

},
dismissButton = {
TextButton(onClick = { showConfirmationDialog = false }) {
Text(text = "No")
Copy link
Collaborator

Choose a reason for hiding this comment

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

use stringResource


Image(
painterResource(R.drawable.mifos_logo),
contentDescription = "",
Copy link
Collaborator

Choose a reason for hiding this comment

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

pass null as contentDesc

.fillMaxSize()) {

Image(
painterResource(R.drawable.mifos_logo),
Copy link
Collaborator

Choose a reason for hiding this comment

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

painter = painterResource(R.drawable.mifos_logo),

Comment on lines 21 to 23
override fun onBackPressed() {
MaterialDialog.Builder().init(this)
.setTitle(getString(R.string.dialog_cancel_registration_title))
.setMessage(getString(R.string.dialog_cancel_registration_message))
.setPositiveButton(getString(R.string.yes)) { _, _ -> super.onBackPressed() }
.setNegativeButton(R.string.no) { dialog, _ -> dialog.dismiss() }
.createMaterialDialog()
.show()
super.onBackPressed()
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

remove this function

var showConfirmationDialog by remember { mutableStateOf(false) }

BackHandler(enabled = true)
{
Copy link
Collaborator

Choose a reason for hiding this comment

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

BackHandler(endabled = true) {

do proper indentation


var temp = true
if (requestID.text.isEmpty()) {
requestIDError = true;
Copy link
Collaborator

Choose a reason for hiding this comment

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

remove semicolon

requestIDError = false
},
label = R.string.request_id,
supportingText = "Request ID cannot be empty",
Copy link
Collaborator

Choose a reason for hiding this comment

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

use stringResource

authenticationTokenError = false
},
label = R.string.authentication_token,
supportingText = "Authentication Token cannot be empty",
Copy link
Collaborator

Choose a reason for hiding this comment

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

use stringResource

},
Modifier
.fillMaxWidth()
.padding(start = 12.dp, end = 16.dp, top = 4.dp),
Copy link
Collaborator

Choose a reason for hiding this comment

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

not start padding, top padding should be 12, keep start 16

@therajanmaurya therajanmaurya merged commit a522937 into openMF:development Jun 13, 2024
0 of 3 checks passed
itsPronay pushed a commit to itsPronay/mifos-mobile that referenced this pull request Jun 14, 2024
* "Refactored registration verification screen to compose"

* "Refactored registration verification screen to compose"

* "Refactored registration verification screen to compose"

* "Added AlertDialog, Component and resolved changes"

* "Implemented Box for Content and Overlayed other states"

* "Added String Resource and Indendtaions"

* "Added String Resource and Indendtaions"
@akashmeruva9 akashmeruva9 changed the title refactor: Registration verification from XML to compose [2656] - refactor: Registration verification from XML to compose Jul 11, 2024
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.

refactor: migration of RegistrationVerificationFragment to compose screen
4 participants