-
Notifications
You must be signed in to change notification settings - Fork 0
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
Profiles SASS rewrite #335
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've gone through and marked the cleanup I plan on doing before merging.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks really good! There are some minor adjustments we can make to get the spacing exactly the same, but we'll have to see how that fits into the responsive sizing changes.
Just made a couple of minor comments about future refactors for where certain markup should live -- Nothing that has to be fixed right away. We can make a tech-debt issue and fix it in a follow-up PR
Follow up PR in #340 |
@pr1sm I'm stumped on where I should add b.m.s in the locationFields fields, or if there's a better location for it. Any advice? |
Have tests to fix and billingMatchesShipping to add back in |
Fixed. Have to port the BMS tests from |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
UI changes look good! There are a couple of follow-up items that need to be split out into separate issues if you don't address them in this PR.
Once you do that, I think this should be good to go
safe to merge? I'll worry about the tests later.. We need to get this UI teaser update out. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changes look good to me! You can merge when ready
name: Profiles SASS rewrite
about: This PR implements rewriting the markup for the profiles page to use a more driven approach using SASS and a flexbox grid
Changes
TOGO
Checks
old
vs.new
and decide if anything needs tweakedfixes #325