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

Bug 1210864 - Add Private Browsing to Firefox Family subnav #3474

Merged

Conversation

malena
Copy link
Contributor

@malena malena commented Oct 26, 2015

@craigcook
Copy link
Member

Should add the link to the iOS subnav as well.

@malena
Copy link
Contributor Author

malena commented Oct 26, 2015

@craigcook Oh I thought that's what you would do? I am confusing what you were going to add the menu.

@craigcook
Copy link
Member

Sorry, I added the iOS top link and subnav previously but couldn't add Private Browsing to the iOS subnav until the page was ready. I'm happy to add it separately once this merges but it doesn't have to wait, it can go into the same commit if you're up for it.

@malena
Copy link
Contributor Author

malena commented Oct 26, 2015

@craigcook Ahh okay no problemo!

@malena malena force-pushed the bug-1210864-add-private-browsing-to-family-nav branch 2 times, most recently from ee3b9a4 to 8bfbbda Compare October 29, 2015 14:12
@malena
Copy link
Contributor Author

malena commented Oct 29, 2015

r?

@malena malena force-pushed the bug-1210864-add-private-browsing-to-family-nav branch from 8bfbbda to a3fb999 Compare October 29, 2015 18:46
@malena
Copy link
Contributor Author

malena commented Oct 29, 2015

Includes a waffle switch. r?

@@ -482,6 +482,9 @@
</li>
<li><a href="{{ url('firefox.sync') }}"{% if activesub == 'sync' %} class="selected"{% endif %}><div>{{ _('Sync') }}</div></a></li>
<li><a href="{{ url('firefox.hello') }}"{% if activesub == 'hello' %} class="selected"{% endif %}><div>{{ _('Hello') }}</div></a></li>
{% if l10n_has_tag('tracking_protection') and waffle.switch('tracking-protection') %}
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove the l10n tag check, only waffle is needed. We can't use tags on a shared thing like navigation.

@malena malena force-pushed the bug-1210864-add-private-browsing-to-family-nav branch 2 times, most recently from 609d9ef to f38163a Compare October 29, 2015 19:03
@jpetto jpetto self-assigned this Oct 29, 2015
@jpetto
Copy link
Contributor

jpetto commented Oct 29, 2015

We knew it would eventually come to this - the subnav is now too long in some locales. Here's a shot of es-ES at tablet viewport:

screen shot 2015-10-29 at 3 50 16 pm

Unless there are other tenable ideas, I think our best option is to drop the sub-sub nav (Trusted/Flexible/Fast) for the desktop pages. Will need to get sign-off from jbertsch & habber.

@jbertsch
Copy link

@malena malena force-pushed the bug-1210864-add-private-browsing-to-family-nav branch from e671f05 to f619a1e Compare October 30, 2015 17:47
@@ -281,7 +281,7 @@
opacity: 0.8;
height: 77px;
line-height: 77px;
max-width: 125px; // uhhh, seems to work?
//max-width: 145px;
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should keep this restriction in place, and should set it to 129px. At tablet width, the #desktop-subnav is 556px wide, which, divided by 4 (the max elements now possible in the subnav) is 139. Each <a> has a left and right padding of 5px, hence 129.

If this restriction isn't in place, text will not wrap to two lines, which means an <li> could get pushed underneath and out of view for some locales.

@jpetto
Copy link
Contributor

jpetto commented Oct 30, 2015

One small CSS issue, but otherwise looking good! r+wc

@malena malena force-pushed the bug-1210864-add-private-browsing-to-family-nav branch from d5c42fa to f2ef5cf Compare November 2, 2015 14:47
@malena
Copy link
Contributor Author

malena commented Nov 2, 2015

@jpetto updated and squashed commits.

@malena malena force-pushed the bug-1210864-add-private-browsing-to-family-nav branch from f2ef5cf to 2143b7d Compare November 2, 2015 14:48
@jpetto
Copy link
Contributor

jpetto commented Nov 2, 2015

Gah, I'm sorry to ask this late, but can we make a special CSS rule for en-* only to make sure "Private Browsing" fits on one line? I know this will be requested later if we don't do it now. (I thought I tested this during my last review, but apparently I was in tablet mode.)

I think we can just add a bit of CSS to line 428:

html[lang^='en'] .fxfamilynav {
    .subnav > li > a {
        max-width: 149px;
    }

    .subsubnav a {
        text-transform: uppercase;
    }
}

@malena malena force-pushed the bug-1210864-add-private-browsing-to-family-nav branch from 2143b7d to 0ad71cd Compare November 2, 2015 17:07
@malena
Copy link
Contributor Author

malena commented Nov 2, 2015

@jpetto no problemo updated!

@jpetto
Copy link
Contributor

jpetto commented Nov 2, 2015

Looks great! Just waiting for Travis... ⌛

jpetto added a commit that referenced this pull request Nov 2, 2015
…-to-family-nav

Bug 1210864 - Add Private Browsing to Firefox Family subnav
@jpetto jpetto merged commit 878bff3 into mozilla:master Nov 2, 2015
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

6 participants