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

Profiles SASS rewrite #335

Merged
merged 52 commits into from
Feb 18, 2019
Merged

Profiles SASS rewrite #335

merged 52 commits into from
Feb 18, 2019

Conversation

walmat
Copy link
Owner

@walmat walmat commented Feb 13, 2019


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

  • Rewrote the profiles page
  • Fixed a few task page sass bugs

TOGO

  • revert navbar and navbar state to original state

Checks

  • CI passes
  • Coverage (<2%∆)
  • Manual Checks
    • View old vs. new and decide if anything needs tweaked

fixes #325

@walmat walmat requested a review from pr1sm February 13, 2019 21:48
Copy link
Owner Author

@walmat walmat left a 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.

packages/frontend/src/app.jsx Outdated Show resolved Hide resolved
packages/frontend/src/app.jsx Outdated Show resolved Hide resolved
packages/frontend/src/navbar/navbar.jsx Outdated Show resolved Hide resolved
packages/frontend/src/navbar/navbar.jsx Outdated Show resolved Hide resolved
packages/frontend/src/navbar/navbar.jsx Outdated Show resolved Hide resolved
packages/frontend/src/profiles/old/locationFields.jsx Outdated Show resolved Hide resolved
packages/frontend/src/profiles/old/paymentFields.jsx Outdated Show resolved Hide resolved
Copy link
Collaborator

@pr1sm pr1sm left a 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

@walmat
Copy link
Owner Author

walmat commented Feb 16, 2019

Follow up PR in #340

@walmat
Copy link
Owner Author

walmat commented Feb 16, 2019

@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?

@walmat
Copy link
Owner Author

walmat commented Feb 16, 2019

Have tests to fix and billingMatchesShipping to add back in

@walmat
Copy link
Owner Author

walmat commented Feb 16, 2019

Fixed. Have to port the BMS tests from profiles.jsx to locationFields.jsx still. Might take me awhile knowing me lol

Copy link
Collaborator

@pr1sm pr1sm left a 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

@walmat
Copy link
Owner Author

walmat commented Feb 18, 2019

safe to merge? I'll worry about the tests later.. We need to get this UI teaser update out.

Copy link
Collaborator

@pr1sm pr1sm left a 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

@walmat walmat merged commit ce2253f into b1.0.0-beta.6 Feb 18, 2019
@walmat walmat deleted the issue_325 branch February 18, 2019 01:34
@walmat walmat mentioned this pull request Feb 27, 2019
5 tasks
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