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

Feat: User details in nav header and New User Profile Activity #78

Merged
merged 1 commit into from Jun 5, 2018

Conversation

Hiteshgautam01
Copy link
Contributor

@Hiteshgautam01 Hiteshgautam01 commented May 25, 2018

Please make sure these boxes are checked before submitting your pull request - thanks!

  • Apply the AndroidStyle.xml style template to your code in Android Studio.

  • Run the checks with ./gradlew check to make sure you didn't break anything

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

@Hiteshgautam01
Copy link
Contributor Author

Hiteshgautam01 commented May 25, 2018

UI after changes

@Override
public void onClick(View v) {
//In this onClick listener we have to go from this UserProfileActivity to myWorkflows fragment.
//Suggest the method to perform this action.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hey @sagar15795 , Suggest the best possible method to perform this action

Copy link
Member

Choose a reason for hiding this comment

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

UserProfile -> myWorkflow and myWorkflow -> UserProfile. So make a separate activity for myworkflow and just show the myworkflow fragment in it.

Copy link
Member

@sagar15795 sagar15795 left a comment

Choose a reason for hiding this comment

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

@Hiteshgautam01 where did you find nav_header.png? What is its license and copyright?

app/build.gradle Outdated
@@ -86,6 +86,7 @@ dependencies {

//Dependencies for butterknife
implementation "com.jakewharton:butterknife:$rootProject.butterKnifeVersion"
implementation 'com.android.support.constraint:constraint-layout:1.1.0'
Copy link
Member

Choose a reason for hiding this comment

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

remove this unused library.

Toolbar mToolbar;

@BindView(R.id.user_name)
TextView user_name;
Copy link
Member

Choose a reason for hiding this comment

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

Use camel notation for the variable name. Also, use m prefix for all class variables.

@Hiteshgautam01 Hiteshgautam01 force-pushed the user_profile_nav branch 2 times, most recently from 22f7f4c to 60b361a Compare June 2, 2018 09:18
Copy link
Member

@sagar15795 sagar15795 left a comment

Choose a reason for hiding this comment

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

@Hiteshgautam01 this PR is not up to the mark. Please change the requested change. And Also, before submitting the PR please read this guideline by ribot which we are trying to follow in this project. And also change only that code which is required not which is not required.

app/build.gradle Outdated
@@ -68,6 +68,7 @@ dependencies {
implementation "com.android.support:support-v4:$rootProject.supportLibraryVersion"
implementation "com.android.support:support-annotations:$rootProject.supportLibraryVersion"

implementation 'com.android.support:design:27.1.0'
Copy link
Member

Choose a reason for hiding this comment

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

We are already using this library.
Check below line.

app/build.gradle Outdated
@@ -86,6 +87,7 @@ dependencies {

//Dependencies for butterknife
implementation "com.jakewharton:butterknife:$rootProject.butterKnifeVersion"
implementation 'com.android.support:support-v4:27.1.1'
Copy link
Member

Choose a reason for hiding this comment

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

We are already using this library.
Check below line.

@@ -1,5 +1,4 @@
<?xml version="1.0" encoding="utf-8"?>
<!--
<?xml version="1.0" encoding="utf-8"?><!--
Copy link
Member

Choose a reason for hiding this comment

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

remove all unwanted changes from the whole file.

@@ -271,6 +284,44 @@ private void signOut() {
finish();
}

private void setNavHeader() {
NavigationView navigationView = findViewById(R.id.nav_view);
Copy link
Member

Choose a reason for hiding this comment

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

use butterknife for this.

@@ -271,6 +284,44 @@ private void signOut() {
finish();
}

private void setNavHeader() {
NavigationView navigationView = findViewById(R.id.nav_view);
View header_view = navigationView.getHeaderView(0);
Copy link
Member

Choose a reason for hiding this comment

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

make it headeView in place of header_view

}
});

myFavoriteWorkflowLayout.setOnClickListener(new View.OnClickListener() {
Copy link
Member

Choose a reason for hiding this comment

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

use butterknife @onClick annotation for click listener.

String city = dataManager.getPreferencesHelper().getUserCity();
String country = dataManager.getPreferencesHelper().getUserCountry();

String avatar = dataManager.getPreferencesHelper().getUserAvatar();
Copy link
Member

Choose a reason for hiding this comment

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

make avatar to avatarUrl

android:layout_centerInParent="true"
android:layout_alignParentTop="true"
app:civ_border_color="@color/white"
/>
Copy link
Member

Choose a reason for hiding this comment

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

move this to above line.

android:layout_below="@+id/user_avatar"
android:layout_centerHorizontal="true"
android:layout_marginTop="@dimen/user_name_margin_top"
/>
Copy link
Member

Choose a reason for hiding this comment

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

move this to above line

android:layout_width="match_parent"
android:layout_height="match_parent"
android:orientation="vertical"
>
Copy link
Member

Choose a reason for hiding this comment

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

move this to above line

@Hiteshgautam01 Hiteshgautam01 force-pushed the user_profile_nav branch 8 times, most recently from 8cbd4f5 to 4181eda Compare June 3, 2018 08:56
Copy link
Member

@sagar15795 sagar15795 left a comment

Choose a reason for hiding this comment

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

@Hiteshgautam01 Please fix this also. Let me know when you will be done with it. So that I can run it on the device before merging to the project.

private void setNavHeader() {

View headerView = navigationView.getHeaderView(0);
String avatar = dataManager.getPreferencesHelper().getUserAvatar();
Copy link
Member

Choose a reason for hiding this comment

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

make avatar to avatarUrl and getUserAvatar() to getUserAvatarUrl()

if (actionbar != null) {
actionbar.setHomeButtonEnabled(true);
actionbar.setDisplayHomeAsUpEnabled(true);
actionbar.setTitle("Favourite Workflows");
Copy link
Member

Choose a reason for hiding this comment

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

move "Favourite Workflows"to string.xml

if (actionbar != null) {
actionbar.setHomeButtonEnabled(true);
actionbar.setDisplayHomeAsUpEnabled(true);
actionbar.setTitle("My Workflows");
Copy link
Member

Choose a reason for hiding this comment

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

move "My Workflows" to string.xml

if (actionbar != null) {
actionbar.setHomeButtonEnabled(true);
actionbar.setDisplayHomeAsUpEnabled(true);
actionbar.setTitle("Profile");
Copy link
Member

Choose a reason for hiding this comment

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

move "Profile" to string.xml


dataManager = new DataManager(new PreferencesHelper(getContext()));

String name = dataManager.getPreferencesHelper().getUserName();
Copy link
Member

Choose a reason for hiding this comment

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

use userName in place of name

android:id="@+id/user_country"
android:layout_width="wrap_content"
android:layout_height="wrap_content"
android:text=""
Copy link
Member

Choose a reason for hiding this comment

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

remove this.

-->
<RelativeLayout xmlns:android="http://schemas.android.com/apk/res/android"
android:layout_width="match_parent"
android:layout_height="178dp"
Copy link
Member

Choose a reason for hiding this comment

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

why 178dp?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@sagar15795 ,Because if I set this to wrap content, the image will stretch in the length and hide the nav drawer due to its size.

Copy link
Member

Choose a reason for hiding this comment

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

Then make it 170dp

android:text="@string/username"
android:textSize="@dimen/tv_textSize"
android:textStyle="bold"
/>
Copy link
Member

Choose a reason for hiding this comment

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

move the close tag above

android:text="@string/email"
android:textSize="@dimen/tv_textSize"
android:textStyle="normal"
/>
Copy link
Member

Choose a reason for hiding this comment

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

move the close tag above

android:layout_marginLeft="@dimen/user_image_margin_left"
android:layout_marginTop="@dimen/user_image_margin_top"
app:civ_border_color="@color/white"
/>
Copy link
Member

Choose a reason for hiding this comment

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

move the close tag above

@Hiteshgautam01 Hiteshgautam01 force-pushed the user_profile_nav branch 3 times, most recently from a413634 to 535a25b Compare June 3, 2018 19:10
Copy link
Member

@sagar15795 sagar15795 left a comment

Choose a reason for hiding this comment

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

@Hiteshgautam01 Please change the requested change and let me know.

TextView UserCity;

@BindView(R.id.user_country)
TextView UserCountry;
Copy link
Member

Choose a reason for hiding this comment

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

make it userCountry or mUserCountry. And all the above also.


private DataManager dataManager;

public static UserProfileFragment newInstance(String id) {
Copy link
Member

Choose a reason for hiding this comment

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

have you used this function? Please delete it if you are not using it.

@asfgit asfgit merged commit e072d3b into apache:master Jun 5, 2018
asfgit pushed a commit that referenced this pull request Jun 5, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants