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

Redesign: Customer profile activity #112

Open
wants to merge 1 commit into
base: redesign
Choose a base branch
from

Conversation

rchtgpt
Copy link
Member

@rchtgpt rchtgpt commented Jan 7, 2020

Fixes Issue

Fixes #105

Screenshot

Screenshot (154)

Description

I have not added the bottom navigation because a lot of other activities would be involved. Hence, I believe addition of bottom navigation bar should be done in a separate PR. Would like to know the views of the project maintainers on it.

  • Run the unit tests 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.

@laxyapahuja
Copy link

hey, I think you missed the navigation drawer icon on the top left, cause it's present in the mock-up.

@luckyman20
Copy link
Collaborator

Try placing the camera icon around the circle image view.

@rchtgpt
Copy link
Member Author

rchtgpt commented Jan 7, 2020

Try placing the camera icon around the circle image view.

@luckyman20 done :)

@laxyapahuja
Copy link

hey @Rachittt, you'll have to add all the hex codes and dimensions in the colors.xml and dimens.xml respectively.

Copy link
Collaborator

@luckyman20 luckyman20 left a comment

Choose a reason for hiding this comment

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

Looks good to me!

@laxyapahuja
Copy link

hey @Rachittt, I think you can add the fonts now as #114 is merged.

@rchtgpt
Copy link
Member Author

rchtgpt commented Jan 27, 2020

hey @Rachittt, I think you can add the fonts now as #114 is merged.

before adding the fonts, I believe it is necessary to get an official approval from the admins on which font should be used for which kind of text, because it should be strictly uniform throughout the application.
@luckyman20 can you please look into it ?

@laxyapahuja
Copy link

hey @Rachittt, I think you can add the fonts now as #114 is merged.

before adding the fonts, I believe it is necessary to get an official approval from the admins on which font should be used for which kind of text, because it should be strictly uniform throughout the application.
@luckyman20 can you please look into it ?

I think we should follow the same font pattern as in the mockups as that is what the mentor approved. Can you let me know what font you used for which kind of text? I've already used the fonts in my PRs. Please review them and let me know which one is wrong as I had to make some guesses because of the size of the mock-up.

@garvit984
Copy link
Collaborator

The text color of the textbox as per the mockup is not grey. There are also some inconsistencies in comparison to original design.

@rchtgpt
Copy link
Member Author

rchtgpt commented May 21, 2020

The text color of the textbox as per the mockup is not grey. There are also some inconsistencies in comparison to original design.

Yeah, during GCI, the project maintainers asked me to make those changes hence you will find inconsistencies in comparison to the original design in almost all redesign PRs, but I am sure those are intentional as long as they don't look aesthetically unappealing.

@garvit984
Copy link
Collaborator

@Rachittt The editext for email and mobile number in these designs asks for input on being clicked.
Please correct it and also the conflicting files.

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.

4 participants