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

HOS_assign #41

Open
wants to merge 8 commits into
base: main
Choose a base branch
from
Open

Conversation

alpesh12345
Copy link
Contributor

Pull request for HOS assignment

Copy link
Contributor

@samuel-abhishek samuel-abhishek left a comment

Choose a reason for hiding this comment

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

Please test the app thoroughly with all possible user scenarios before commit

ohos:below="$id:a"
ohos:orientation="horizontal"
>
<RadioButton
Copy link
Contributor

Choose a reason for hiding this comment

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

Incorrect way to use the radio button for this use case. This way of implementation will let you select both Male & Female. (Suggestion : Use RadioContainer to group the radio buttons)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Implemented click listener in SignUp class to set other false if pressed

TextField mobile = (TextField) findComponentById(ResourceTable.Id_mobile);
RadioButton male = (RadioButton) findComponentById(ResourceTable.Id_gender_male);
RadioButton female = (RadioButton) findComponentById(ResourceTable.Id_gender_female);

Copy link
Contributor

Choose a reason for hiding this comment

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

SignupButton validation is missing.. For the signup button, validate if all the above fields are entered properly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added SignUp validation

if (!name.matches("^[a-zA-Z]*$")) {
//firstname.setText("");
Text er1 = (Text) findComponentById(ResourceTable.Id_er1);
er1.setVisibility(Component.VISIBLE);
Copy link
Contributor

@samuel-abhishek samuel-abhishek Jun 6, 2021

Choose a reason for hiding this comment

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

"er1" must have a null check as setFocusChangedListener is being used.!! [App crash possible scenario : If user clicks on the textfield with setFocusChangedListener and then clicks back button. ] Make sure all the error fields in this class are having the null checks as they are inflated inside the focusChangedListener

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Checked

email.setFocusChangedListener((component, isFocused) -> {
String name = email.getText();
if (!isFocused) {
if (!name.matches("^[a-zA-Z0-9+_.-]+@[a-zA-Z0-9.-]+$")) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Do not use hardcoded string the code. Use Constants

String name = firstname.getText();
if (!isFocused) {
if (!name.matches("^[a-zA-Z]*$")) {
//firstname.setText("");
Copy link
Contributor

@samuel-abhishek samuel-abhishek Jun 6, 2021

Choose a reason for hiding this comment

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

Delete the useless code.(Applies to all files)

@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

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.

3 participants