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

Buyers Guide: addressed nav qa issues #1943

Merged
merged 20 commits into from Oct 17, 2018
Merged

Buyers Guide: addressed nav qa issues #1943

merged 20 commits into from Oct 17, 2018

Conversation

mmmavis
Copy link
Collaborator

@mmmavis mmmavis commented Oct 15, 2018

Closes #1893

Review without whitespace changes:
https://github.com/mozilla/foundation.mozilla.org/pull/1943/files?w=1

Note: share icon functionality will be implemented in a follow-up PR. This PR only covers the UI part of it.

@cadecairos cadecairos temporarily deployed to foundation-mofostaging-pr-1943 October 15, 2018 23:32 Inactive
@mmmavis mmmavis temporarily deployed to foundation-mofostaging-pr-1943 October 16, 2018 00:08 Inactive
@mmmavis mmmavis temporarily deployed to foundation-mofostaging-pr-1943 October 16, 2018 07:38 Inactive
@mmmavis mmmavis temporarily deployed to foundation-mofostaging-pr-1943 October 16, 2018 16:32 Inactive
@kristinashu
Copy link

kristinashu commented Oct 16, 2018

Design review:

  • Reduce size of Donate and Share icons (button is H28px) (share icons are around H20px)
  • Main nav should NOT be sticky, only the sub nav should be sticky - maybe check-in with Pomax about this because he's doing something related on the home page
  • Add link share icon icon-link.svg.zip

Mobile overlay:

  • share icon turns to close icon when open
  • bg gray #F6F6F6
  • share text is .body-large from style guide
  • Donate text heading is .h3-heading from style guide (but white and center aligned)

image

image

@mmmavis mmmavis temporarily deployed to foundation-mofostaging-pr-1943 October 16, 2018 18:19 Inactive
@mmmavis mmmavis temporarily deployed to foundation-mofostaging-pr-1943 October 16, 2018 19:28 Inactive
@mmmavis mmmavis temporarily deployed to foundation-mofostaging-pr-1943 October 16, 2018 19:31 Inactive
@mmmavis mmmavis temporarily deployed to foundation-mofostaging-pr-1943 October 16, 2018 20:01 Inactive
@@ -0,0 +1,56 @@
<div id="primary-nav-container">
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

following the code hierarchy pattern we have for the main Foundation site nav (network-api/networkapi/templates/pages/primary_nav.html)

@@ -1,16 +1,13 @@
.moz-logo {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

cleaned up this file a bit by moving relevant code to primary-nav/primary-nav.scss

@mmmavis mmmavis temporarily deployed to foundation-mofostaging-pr-1943 October 16, 2018 21:40 Inactive
@mmmavis mmmavis changed the title (WIP) BG - nav qa BG - nav qa Oct 16, 2018
@mmmavis mmmavis changed the title BG - nav qa Buyers Guide: addressed nav qa issues Oct 16, 2018
@mmmavis mmmavis temporarily deployed to foundation-mofostaging-pr-1943 October 16, 2018 21:55 Inactive
@kristinashu
Copy link

Awesome! Desktop nav is missing the new link share icon but that's not a blocker.

@mmmavis mmmavis temporarily deployed to foundation-mofostaging-pr-1943 October 16, 2018 22:22 Inactive
Pomax
Pomax previously requested changes Oct 16, 2018
<div class="col">
<h3 class="h3-heading">Help us keep the Internet healthy</h3>
<p>Mozilla is a nonprofit organization fighting for a healthy interent.</p>
<a class="btn btn-donate m-0 w-100" href="https://donate.mozilla.org">Donate</a>
Copy link
Contributor

Choose a reason for hiding this comment

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

does this need the same utm query args as the footer?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@Pomax I'm not sure - this chunk of code was already in the codebase. I only moved it to this file. Let's file a followup ticket to get clarification then.

Copy link
Contributor

Choose a reason for hiding this comment

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

filed #1959

<div class="narrow-screen-menu-background">
<div class="narrow-screen-menu-container">
<div class="social d-flex flex-column">
<a class="social-button body-large social-button-fb" href="#TODO">Facebook</a>
Copy link
Contributor

Choose a reason for hiding this comment

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

can we get away with just not setting an href at all?

@@ -0,0 +1,37 @@
<?xml version="1.0" encoding="utf-8"?>
Copy link
Contributor

Choose a reason for hiding this comment

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

you'll want to run this through svgo, because this is probably <300byte after svg optimization

@mmmavis mmmavis removed the request for review from alanmoo October 16, 2018 23:11
@mmmavis mmmavis temporarily deployed to foundation-mofostaging-pr-1943 October 16, 2018 23:24 Inactive
@mmmavis mmmavis temporarily deployed to foundation-mofostaging-pr-1943 October 16, 2018 23:45 Inactive
@mmmavis mmmavis temporarily deployed to foundation-mofostaging-pr-1943 October 16, 2018 23:48 Inactive
@mmmavis mmmavis requested a review from Pomax October 16, 2018 23:49
@mmmavis mmmavis mentioned this pull request Oct 17, 2018
@mmmavis mmmavis temporarily deployed to foundation-mofostaging-pr-1943 October 17, 2018 16:39 Inactive
@mmmavis mmmavis temporarily deployed to foundation-mofostaging-pr-1943 October 17, 2018 16:54 Inactive
@mmmavis mmmavis temporarily deployed to foundation-mofostaging-pr-1943 October 17, 2018 18:31 Inactive
@mmmavis
Copy link
Collaborator Author

mmmavis commented Oct 17, 2018

@Pomax PR updated. Note that sharing icon functionality related stuff will be handled in #1899 .

Copy link
Contributor

@Pomax Pomax left a comment

Choose a reason for hiding this comment

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

arrrr+

@Pomax Pomax merged commit f6cf4df into master Oct 17, 2018
@Pomax Pomax deleted the issue-1893-bg-nav-qa branch October 17, 2018 19:29
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

4 participants