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

Before you begin 1077 #1110

Merged
merged 7 commits into from
Jan 22, 2024
Merged

Before you begin 1077 #1110

merged 7 commits into from
Jan 22, 2024

Conversation

davidwiese
Copy link
Member

Fixes #1077

Changes made

  • Updated copy on page to match figma
  • Added Keep your browser open alert section
  • Updated spacing between alert and buttons on mobile

Screenshots of proposed changes

Desktop

Screen Shot 2024-01-17 at 12 45 04 PM

Mobile

Screen Shot 2024-01-17 at 12 44 32 PM

@davidwiese davidwiese linked an issue Jan 17, 2024 that may be closed by this pull request
3 tasks
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 @davidwiese ! This looks great! My only request is that we import and use a material UI icon instead of importing one.

Check out this one.
Check out the Advice.tsx for some insight on how to style and use the icons.

@davidwiese
Copy link
Member Author

davidwiese commented Jan 17, 2024

Hey @davidwiese ! This looks great! My only request is that we import and use a material UI icon instead of importing one.

Check out this one. Check out the Advice.tsx for some insight on how to style and use the icons.

@sydneywalcoff
All finished for re-review

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.

Just remove the alert image and we're good!

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 and we're good! :)

Copy link
Member Author

Choose a reason for hiding this comment

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

@sydneywalcoff Woops. All set!

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.

For realsies, last thing lol @davidwiese

Comment on lines 50 to 58
[breakpoints.down('sm')]: {
marginTop: 40,
},

[breakpoints.down('xs')]: {
marginTop: 40,
},
},

Copy link
Member

@sydneywalcoff sydneywalcoff Jan 17, 2024

Choose a reason for hiding this comment

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

@davidwiese jk one last thing. Is this supposed to be here? I'm not seeing a button container on the Before you Begin page, and we don't want unintended consequences on the rest of the buttons?

Copy link
Member Author

@davidwiese davidwiese Jan 17, 2024

Choose a reason for hiding this comment

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

@sydneywalcoff
Yea, that's adjusting the margin between the Before You Begin section and the Buttons on the bottom of the page for mobile layouts (part of the <FlowNavigation /> component).

Copy link
Member Author

Choose a reason for hiding this comment

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

@sydneywalcoff Ok I was just overthinking the problem, or was just misunderstanding the goal of the design change.

My initial understanding was that the buttons should be a fixed distance from the alert, when really it's just the minimum distance that is desired.

Should be good to go now!

@davidwiese davidwiese added the ready for dev lead task ready for dev lead to evaluate label Jan 19, 2024
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.

@davidwiese just a tiny thing and then I'll merge

@@ -48,6 +48,7 @@ const useUtilityStyles = makeStyles<Theme>(
flexDirection: 'row',
width: '100%',
},

Copy link
Member

Choose a reason for hiding this comment

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

@davidwiese seems silly, but lets remove this added line so you aren't making changes to this file at all.

@sydneywalcoff sydneywalcoff removed the ready for dev lead task ready for dev lead to evaluate label Jan 20, 2024
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.

@davidwiese looks good!! I'll merge now

remember to re-add the ready for dev lead label when its ready again after corrections.

@sydneywalcoff sydneywalcoff merged commit 943ade9 into dev Jan 22, 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.

Before you begin
2 participants