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

Dance page - style Amazon logo section #25950

Merged
merged 7 commits into from Nov 9, 2018
Merged

Conversation

nkiruka
Copy link
Contributor

@nkiruka nkiruka commented Nov 9, 2018

This PR is to style the amazon log section

Implementation

  • Make section responsive down to 320px
  • Remove colon from In partnership with
  • Resized logo to 250 px
  • Add space between In partnership with and logo
  • line up text with logo

screen shot

screen shot 2018-11-09 at 9 49 50 am

  • Before

screen shot 2018-11-09 at 4 09 40 am

  • Before (at smaller width)

screen shot 2018-11-09 at 4 09 50 am

  • After

screen shot 2018-11-09 at 3 53 24 am

-After (at smaller width)

screen shot 2018-11-09 at 9 55 44 am

@nkiruka nkiruka requested a review from Erin007 November 9, 2018 17:57
@tanyaparker
Copy link
Contributor

Your checklist in your PR description is unchecked. Are you still working on things or is it ready for review? For example, you said you removed the colon but the After screenshot still has it.

.sponsor-logo-with-text .sponsor-logo-image {
margin-top: 10px;
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: Missing new line at the end of the file

%p= I18n.t(:hoc2018_dance_amazon_sponsor_section_with_logo_title)
%div
%img{src:"/images/fit-450/AFE-Dark-Orange-logo.png"}
%div.sponsor-logo-with-text{style: "font-size: 24px; text-align: center; margin-top: 60px"}
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: I like to keep my HAML as clean as possible. If there is an id or class, you can remove the %div part.

Also, these in-line styles can be moved to your css file.

Copy link
Contributor

Choose a reason for hiding this comment

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

Same for line 6 and throughout the file.

@nkiruka
Copy link
Contributor Author

nkiruka commented Nov 9, 2018

@tanyaparker It will be visible after content scoop today at noon. I will update PR with screen shot. The i18n gsheet has been updated.

@tanyaparker
Copy link
Contributor

You could either 1) ssh into staging, scoop it, merge staging into your branch again or 2) temporarily edit your pegasus/cache/i18n/en-US.yml file with the same change but don't commit it so that you can preview your changes locally.

So are you waiting to check off the checklist until after you merge your PR?

@nkiruka
Copy link
Contributor Author

nkiruka commented Nov 9, 2018

@tanyaparker updated i18n strings have been merged onto staging, updated code per feedback. Ready for review. Thanks

margin-right: 18%;
}

@media screen and (min-width: 0px) and (max-width: 512px) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think you need to have a separate css file just for the logo classes. I'd put this inside the dance.css file which should have all the styles needed for /dance.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done!

@@ -1,9 +1,10 @@
%link{:rel=>'stylesheet', :type=>'text/css', :href=>'/css/dance-landing/dance-sponsor-logo.css'}
%link{href: "/css/dance-landing/dance.css", type: "text/css", rel: "stylesheet"}
Copy link
Contributor

Choose a reason for hiding this comment

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

You don't need to load this again since it's already loaded on dance.haml.

@nkiruka nkiruka merged commit 99bf0c5 into staging Nov 9, 2018
@nkiruka nkiruka deleted the amazon-logo-responsive branch November 9, 2018 21:00
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.

None yet

3 participants