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

Update alignment on Primary & Bannered Campaign Pages #3025

Merged
merged 10 commits into from
Apr 24, 2019

Conversation

@cadecairos cadecairos temporarily deployed to foundation-mofostaging-pr-3025 April 16, 2019 22:49 Inactive
@mmmavis mmmavis changed the title Update alignment on Primary & Bannered Campaign Pages (WIP) Update alignment on Primary & Bannered Campaign Pages Apr 16, 2019
@mmmavis mmmavis temporarily deployed to foundation-mofostaging-pr-3025 April 16, 2019 23:52 Inactive
@mmmavis mmmavis temporarily deployed to foundation-mofostaging-pr-3025 April 17, 2019 20:06 Inactive
@mmmavis mmmavis requested a review from beccaklam April 18, 2019 16:39
@mmmavis mmmavis changed the title (WIP) Update alignment on Primary & Bannered Campaign Pages Update alignment on Primary & Bannered Campaign Pages Apr 18, 2019
@mmmavis mmmavis requested a review from youriwims April 18, 2019 16:41
Copy link

@beccaklam beccaklam left a comment

Choose a reason for hiding this comment

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

Hey @mmmavis the changes look great! Only thing is did we say whether or not we're removing that line we talked about? It looks a bit funny when there's no subnav:
Screenshot_2019-04-18 Narrow primary page
Screenshot_2019-04-18 Narrow primary page(1)

@beccaklam
Copy link

Can we also increase the 15px padding on the right side to 24px?
Screen Shot 2019-04-18 at 2 16 26 PM

@mmmavis
Copy link
Collaborator Author

mmmavis commented Apr 18, 2019

@kristinashu ^ regarding the grey line

@kristinashu
Copy link

So I think we should keep the 15px on the right to allow for longer line length, I think it's rare that a letter will go right to the right edge.

Yes! Go ahead and remove that mysterious grey line!

@mmmavis
Copy link
Collaborator Author

mmmavis commented Apr 22, 2019

@beccaklam if it works for you - I'm gonna file a follow-up for removing that grey line as it's a more complicated fix and will affect implementation of other page types as well.

@beccaklam
Copy link

@beccaklam if it works for you - I'm gonna file a follow-up for removing that grey line as it's a more complicated fix and will affect implementation of other page types as well.

That works for me!

@cadecairos cadecairos temporarily deployed to foundation-mofostaging-pr-3025 April 22, 2019 20:47 Inactive
@mmmavis
Copy link
Collaborator Author

mmmavis commented Apr 22, 2019

Follow-up ticket filed: #3047

@mmmavis mmmavis changed the title Update alignment on Primary & Bannered Campaign Pages (WIP) Update alignment on Primary & Bannered Campaign Pages Apr 22, 2019
@beccaklam
Copy link

Hey @mmmavis so for this PR, is the spacing fixed with your PR 3049? There should be about 24px between the intro paragraph and the menu:

PR:

Screen Shot 2019-04-23 at 10 21 02 AM

Design:

Screen Shot 2019-04-23 at 10 49 11 AM

@mmmavis
Copy link
Collaborator Author

mmmavis commented Apr 23, 2019

@beccaklam no this PR is not up-to-date with master (aka with changes from other merged PRs) yet. It turns out this work is more complicated than I expected 'cuz the code is heavily shared between all page types. To avoid confusion, I'm gonna remove PR reviewers first and assign you back once it's ready for review again.

@mmmavis mmmavis removed the request for review from youriwims April 23, 2019 16:46
@beccaklam
Copy link

@mmmavis ok! 👌

@mmmavis mmmavis temporarily deployed to foundation-mofostaging-pr-3025 April 23, 2019 22:11 Inactive
@mmmavis mmmavis temporarily deployed to foundation-mofostaging-pr-3025 April 23, 2019 23:49 Inactive
@mmmavis mmmavis temporarily deployed to foundation-mofostaging-pr-3025 April 23, 2019 23:52 Inactive
@mmmavis mmmavis temporarily deployed to foundation-mofostaging-pr-3025 April 24, 2019 00:22 Inactive
@mmmavis mmmavis temporarily deployed to foundation-mofostaging-pr-3025 April 24, 2019 00:24 Inactive
@mmmavis mmmavis changed the title (WIP) Update alignment on Primary & Bannered Campaign Pages Update alignment on Primary & Bannered Campaign Pages Apr 24, 2019
@mmmavis
Copy link
Collaborator Author

mmmavis commented Apr 24, 2019

@beccaklam This is ready for design review! Phew! I've included fixes for #3045 as well... hopefully I have captured everything.

Once this passes design review I'll assign dev for code review.

@mmmavis mmmavis requested a review from beccaklam April 24, 2019 00:35
Copy link

@beccaklam beccaklam left a comment

Choose a reason for hiding this comment

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

This all looks amazing!!! 💯 💯 💯 Good to go from design 👍 Awesome job Mavis 🙌

@mmmavis mmmavis requested a review from youriwims April 24, 2019 16:46
@mmmavis
Copy link
Collaborator Author

mmmavis commented Apr 24, 2019

@youriwims this is ready for code review! 👀

Copy link
Contributor

@youriwims youriwims left a comment

Choose a reason for hiding this comment

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

lgtm!

@mmmavis
Copy link
Collaborator Author

mmmavis commented Apr 24, 2019

Thanks @beccaklam @youriwims 🎉 I know this is a big PR to review!

@mmmavis mmmavis merged commit 6609e25 into master Apr 24, 2019
@mmmavis mmmavis deleted the issue-3022-primary-bannered-cutout branch April 30, 2019 23:28
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.

Update Alignment on Primary & Bannered Campaign Pages
5 participants