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

#955 - Improved the progress bar to match figma #1040

Merged
merged 9 commits into from
Feb 1, 2024

Conversation

ramankala
Copy link
Member

Refactored the progress bar so that the UI would look better for the user to look at.

#955

Refactored the progress bar so that the UI would look better for hte user to look at.

#955
Copy link
Member

@sydneywalcoff sydneywalcoff left a comment

Choose a reason for hiding this comment

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

Hey Raman!

Can you refactor this to use material UI for styling?

@@ -167,7 +168,7 @@ const FormHeader = () => {
return (
<div className={classes.outerWrapper}>
<div className={classes.formHeader}>
<h3>{formTitle}</h3>
<h3 className="formTitle">{formTitle}</h3>
Copy link
Member

Choose a reason for hiding this comment

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

rather than importing an external css file and creating a 'formTitle' class inline, can you use Material UI and create a class that way?

Copy link
Member

Choose a reason for hiding this comment

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

lets remove this and use material ui to style it.

Copy link
Member

@sydneywalcoff sydneywalcoff left a comment

Choose a reason for hiding this comment

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

Hey @ramankala! Desktop looks great! Great job translating to material ui.

The progress bar should get a little smaller as the screen sizes get smaller. I noticed the original figma link I gave you doesn't work anymore, let me know if this is ever a problem again.

for the small breakpoint and below, can you reduce the padding/margin a little bit?

Copy link
Member

@sydneywalcoff sydneywalcoff left a comment

Choose a reason for hiding this comment

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

@ramankala sorry I should have been clearer. I would like the size of the progress bar to remain the same at desktop, but at smaller screen sizes, I'd like to see these changes.

Copy link
Member

@sydneywalcoff sydneywalcoff left a comment

Choose a reason for hiding this comment

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

Hey @ramankala!

Check #955 for the figma that design added before the break. There's a couple inconsistencies between your branch and the figma.

Copy link
Member

@sydneywalcoff sydneywalcoff left a comment

Choose a reason for hiding this comment

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

Hey @ramankala,

Just a few comments below. I was also experiencing a responsiveness bug, but I think addressing the issues below might fix it.

borderBottomLeftRadius: '64px',
maxWidth: '80%',
marginLeft: '10%',
'@media (max-height:759px)': {
Copy link
Member

Choose a reason for hiding this comment

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

can you refactor this to use breakpoints.down rather than this template literal?

Copy link
Member

Choose a reason for hiding this comment

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

Also, should this be max-width instead of max-height?

Copy link
Member

@sydneywalcoff sydneywalcoff left a comment

Choose a reason for hiding this comment

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

@ramankala I know you didn't get a tablet design. But between 400-767px, there are points when the progress bar is shorter than the rest of the content.
Screen Shot 2024-01-22 at 7 05 36 PM

Can you apply the mobile styles (where the progress bar goes all the way across) starting at 767?

Copy link
Member

@sydneywalcoff sydneywalcoff left a comment

Choose a reason for hiding this comment

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

@ramankala Visually, its perfect! Great job!

I just need you to run prettier using npm run lint:fix and we can get this merged 🎉

Copy link
Member

@sydneywalcoff sydneywalcoff 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 @ramankala !! 🎉

@sydneywalcoff sydneywalcoff merged commit 111cef8 into dev Feb 1, 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.

2 participants