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

Fix: profile ui improvements #372

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

Conversation

rajatgupta24
Copy link
Contributor

Issue Number

fixes #
UI Improvements in profile section

  • Center aligned the activity, achievement bar
  • Decrease the height of cards in mobile view
  • Replace the "view" with ">"

Describe the changes you've made

Describe if there is any unusual behavior (Any Warning) of your code(Write NA if there isn't)

Additional context (OPTIONAL)

Test plan (OPTIONAL)

A good test plan should give instructions that someone else can easily follow.

Checklist

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • All new and existing tests passed.
  • The title of my pull request is a short description of the requested changes.

Provide a Deployed link of route/page that needs to review
Preview: Deploy preview link here with the appropriate route

@Abhishek-kumar09
Copy link
Contributor

Please review @adarsh-technocrat

@adarsh-technocrat
Copy link
Collaborator

Please review @adarsh-technocrat

Sure @Abhishek-kumar09 :)

Comment on lines 77 to 80
},
bar: {
width: "33%",
},
Copy link
Collaborator

Choose a reason for hiding this comment

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

Hey, @rajatgupta24 for mobile view the tab section is looking grate but by defining the width the tab component has lost its responsiveness as a tab variant is scrollable. so instead of defining the width, we can use the native CSS class for the component by that it will not lose its responsiveness. and can we reduce the width on the web and tab around 15px by adding breakpoints?

fix

Copy link
Collaborator

@adarsh-technocrat adarsh-technocrat Jul 1, 2021

Choose a reason for hiding this comment

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

sss

we can reduce the gap here!

Comment on lines 11 to 23
<Card className={classes.card}>
<CardContent className={classes.cardContent}>
<img src={icon} className={classes.image} />
<Typography variant="h6" color="textSecondary" gutterBottom className={classes.title}>
{title}
</Typography>
<div className={classes.titleCont}>
<Typography variant="h6" color="textSecondary" gutterBottom className={classes.title}>
{title}
</Typography>
<Typography variant="body2" gutterBottom>
<Link>
<KeyboardArrowRightIcon style={{ marginLeft: ".05rem" }} />
</Link>
</Typography>
</div>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Hey, @rajatgupta24 I think we can revert this change and go with the previous one as it's kind of breaking the UI of the Activity card and the placement of the arrow doesn't seem right!

Capture11

Comment on lines 12 to 14
<Typography className={classes.title} variant="h2" color="textPrimary">
{/* <Typography className={classes.title} variant="h2" color="textPrimary">
Profile & settings
</Typography>
</Typography> */}
Copy link
Collaborator

@adarsh-technocrat adarsh-technocrat Jul 1, 2021

Choose a reason for hiding this comment

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

@rajatgupta24 please remove this comment! and related CSS implementation if there. and @rajatgupta24 small suggestion if you make any changes in any component please check for its responsiveness for all screens =)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @adarsh-technocrat, should I add the "profile & setting" heading again, I removed it.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Hi @adarsh-technocrat, should I add the "profile & setting" heading again, I removed it.

@rajatgupta24 well it's looking good remove that!

Copy link
Collaborator

Choose a reason for hiding this comment

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

Hey, @rajatgupta24 keep this "profile & setting" I have made few changes to it !!

Copy link
Contributor

@Abhijay007 Abhijay007 left a comment

Choose a reason for hiding this comment

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

@rajatgupta24 I reviewed your code and there are some changes that I want to suggest

  1. the nav is perfectly center for the mobile view but its alignment gets affected in the web view which looks a bit distorted (not centered not left).

nav

  1. I think that the "view" button looks better than the "forward arrow" in web view but it looks fine in mobile view.

florward arrow

  1. I think you mistakenly commented "Profile and settings " heading on the profile page

@Abhijay007
Copy link
Contributor

Abhijay007 commented Jul 1, 2021

@adarsh-technocrat I mistakenly reviewed it too 😅 I don't know that you already reviewed this, maybe I was typing that time sorry my bad :(

@rajatgupta24
Copy link
Contributor Author

Hey @adarsh-technocrat,
I made the changes, & centered the achievement tabs at tablet view (still scrollable).

And the changes in package & package-lock.json are because I ran npm audit fix.

@rajatgupta24
Copy link
Contributor Author

Desktop View

image
Tablet View

image
Mobile View

image

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