Fix bug 1434558, swap search and toolbox components #4656

schalkneethling · 2018-02-08T14:49:11Z

This is to swap the toolbox and search components in the site header. This also required a number of changes to the header overall.

So far I have tested this in:

Chrome
Firefox
Safari

Using both

en-US
fa

To-Do

Test in:

IE 11
Edge
IE 10 (as a basic sanity check)

Code is good to review. Thanks!

jwhitlock · 2018-02-09T17:32:54Z

Thanks @schalkneethling. This looks like a good refactoring of the base template, although I'll need to look closer to see exactly what moved where. Will Feb 13 work for a detailed review, or do you need it sooner?

schalkneethling · 2018-02-09T19:23:24Z

@jwhitlock That works for me. Thanks!

stephaniehobson · 2018-02-09T19:46:27Z

kuma/static/js/components/nav-main-search.js

+    var navMainSearchForm = document.getElementById('nav-main-search');
+    var searchInput = document.getElementById('main-q');
+    var searchTrigger = navMainSearchForm.querySelector('.search-trigger');
+    var searchWrap = navMainSearchForm.querySelector('.search-wrap');


Nice modern JS.

stephaniehobson · 2018-02-09T19:50:48Z

kuma/static/styles/components/structure.scss

-@import 'structure/main-header';
-@import 'structure/submenu';
+@import 'structure/page-header/main-header';
+@import 'structure/page-header/submenu';


Good structural changes :)

If you want to preserve the git history for these moved pages you can move them in a separate PR where you don't edit the files. This might be complicated rebase dance so it's not a blocker.

stephaniehobson

I like how you split some of the templates up into smaller macros :) But I'm not seeing the translated strings show up in my local dev anymore. @jwhitlock Is this something that will be fixed after the strings are scraped or will they need to be translated again?

The navigation menu names are longer in some locales and I think there are problems at around 1024px in German when logged in. (This is true of what's in production though so maybe not a blocker.)

The entire black square around the search icon needs to be clickable. And you can fix Bug 1384653!

"Skip to search" in the a11y nav menu should trigger the animation and put focus on the search field.

I can't tab to the search field anymore.

I think the above two things used to work because the input field did not have a aria-hidden="true" value on it so it could be focused. If you want to change to require interaction with the search icon to trigger the field you should make that span into a button.

I'm glad to see this change happening! It's a real improvement over what's there now.

stephaniehobson · 2018-02-09T19:54:51Z

kuma/static/styles/components/structure/page-header/login.scss


        > a {
-            color: #fff;
+            color: #333;


$text-color variable?

stephaniehobson · 2018-02-09T19:55:14Z

kuma/static/styles/components/structure/page-header/login.scss

    }

    .login-photo {
+        border-radius: 50%;


Round photo 💯

stephaniehobson · 2018-02-09T19:57:23Z

kuma/static/styles/components/structure/page-header/nav-toolbox-wrapper.scss

+        border-top: 5px solid $grey-light;
+
+        .toolbox {
+            text-align: right;


stephaniehobson · 2018-02-09T20:21:14Z

kuma/static/styles/components/structure/page-header/nav-main-search.scss

+    }
+
+    input[type='search'] {
+        color: #333;


$text-color?

stephaniehobson · 2018-02-09T20:22:41Z

kuma/static/styles/components/structure/page-header/nav-main-search.scss

+        ));
+        @include bidi((
+            (right, auto, auto),
+        ));


These declarations should be inside the same mixin:

@include bidi(( (left, 30%, 30%), (right, auto, auto), ));

stephaniehobson · 2018-02-09T20:23:52Z

kuma/static/styles/components/structure/page-header/submenu.scss

+    ));
+    @include bidi((
+        (text-align, left, right),
+    ));


These should be combined:

@include bidi(( (right, 0, left, 0), (text-align, left, right), ));

stephaniehobson · 2018-02-09T20:24:15Z

kuma/static/styles/components/structure/page-header/submenu.scss

    box-shadow: 0 4px 4px rgba(0, 0, 0, .15);
    background-color: #fff;
+    @include bidi((
+        (right, 0, left, 0),


Right needs to be set to auto when left is set.

(right, 0, left, auto),

stephaniehobson · 2018-02-09T20:24:57Z

kuma/static/styles/components/structure/page-header/toolbox.scss

+            @include large-inline-nav-item();
+
+            > a {
+                color: #333;


$text-color?

stephaniehobson · 2018-02-09T20:25:05Z

kuma/static/styles/components/structure/page-header/toolbox.scss

+
+                &:hover,
+                &:focus {
+                    color: #333;


$text-color?

stephaniehobson · 2018-02-09T20:35:43Z

This should also be tested with a touch screen, or at least a touch screen emulator :)

schalkneethling · 2018-02-12T09:41:20Z

Thank you for the review @stephaniehobson - I still need to update the PR but, I have addressed most of the items here except:

The entire black square around the search icon needs to be clickable. And you can fix Bug 1384653!
"Skip to search" in the a11y nav menu should trigger the animation and put focus on the search field.
I can't tab to the search field anymore.

I will tick these of as I address them, and update the PR.

codecov-io · 2018-02-12T12:48:06Z

Codecov Report

Merging #4656 into master will decrease coverage by <.01%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #4656      +/-   ##
==========================================
- Coverage   95.27%   95.27%   -0.01%     
==========================================
  Files         261      260       -1     
  Lines       23571    23554      -17     
  Branches     1692     1692              
==========================================
- Hits        22458    22441      -17     
  Misses        902      902              
  Partials      211      211

Impacted Files	Coverage Δ
kuma/settings/common.py	`92.57% <ø> (ø)`	⬆️
kuma/wiki/tests/test_views_document.py	`99.27% <0%> (-0.02%)`	⬇️
kuma/wiki/jobs.py	`97.29% <0%> (ø)`	⬆️
kuma/wiki/tests/test_signals.py

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2871015...1613744. Read the comment docs.

schalkneethling · 2018-02-12T13:57:00Z

IE 11 is turning out to be a heck of a thing to test. Seems like VirtualBox does not play nice with High Sierra :-/ From the bit I could test so far, it seems these media queries[1] are not being applied in IE11

[1] https://github.com/mozilla/kuma/pull/4656/files#diff-db0e18eb73f5b50ef87c887f6567253fR137

schalkneethling · 2018-02-12T16:00:12Z

Ok, I know what the problem is with IE, will update the PR momentarily.

jwhitlock · 2018-02-12T16:38:31Z

@stephaniehobson I can't reproduce the issue with locales, they work for me:

Some things to try:

Is the locale/ folder present? A git submodule update --init would fix it.
Re-compile locales with docker exec -i kuma_web_1 make localecompile

jwhitlock

I think @stephaniehobson's review is sufficient for the front end changes.

The hashed assets build, so you got the settings right ✅

Here's an interesting interaction (Firefox, Chrome) that might be fixable:

Click in search. It hides the login area and expands
Click below or above the search text input. The search box starts to collapse, then bounces back open. The login area is exposed again.

The functional tests for the search header are broken. They expect that the menus (Technologies, References & Guides, Feedback) will be hidden when the search box is exposed. This could be updated to expect the login area to be hidden. @schalkneethling, do you want to get into Kuma functional testing, or do you want me to add a commit?

jwhitlock · 2018-02-12T16:48:51Z

tests/pages/base.py

        report_bug_form_url = 'https://bugzilla.mozilla.org/form.mdn'
        # locators
-        SIGNIN_SELECTOR = '#nav-sec .login-link'
+        SIGNIN_SELECTOR = '#toolbox .login-link'


👍 login link tests work

schalkneethling · 2018-02-13T11:14:26Z

@jwhitlock Fixed the problem you identified. I tried my hand at running the tests locally but, I keep running into walls with various Python and virtualenv error. In the interest of time, I would appreciate it if you could commit the fix for the failing test(s) - Thanks!

jwhitlock · 2018-02-13T13:57:18Z

No problem, happy to do it.

Next time, you might try:

scripts/run_functional_tests.sh tests/functional/test_search.py

This runs the tests inside Docker, avoiding local setup of Python etc.

jwhitlock · 2018-02-13T18:08:38Z

@stephaniehobson, this looks good to me now. Do you want to have another look before merging?

jwhitlock · 2018-02-13T18:11:08Z

Ah, there are some interesting breakpoints in German:

schalkneethling · 2018-02-13T19:35:16Z

Ah nuts! How did I forget to test German - Will address this and update the PR. Thanks @jwhitlock

stephaniehobson · 2018-02-13T22:18:42Z

Mobile Safari is putting the rounded corners on the search field 😖

All the interactions work with touch ☝️

schalkneethling · 2018-02-14T14:01:35Z

Ok, both of the items raised by @jwhitlock and @stephaniehobson has been resolved. Thanks a lot for the thorough review o/\o

jwhitlock · 2018-02-14T15:57:01Z

New "middle" layout works for German, Russian, etc.

schalkneethling requested review from jwhitlock and stephaniehobson February 8, 2018 14:49

stephaniehobson reviewed Feb 9, 2018

View reviewed changes

stephaniehobson suggested changes Feb 9, 2018

View reviewed changes

jwhitlock reviewed Feb 12, 2018

View reviewed changes

jwhitlock approved these changes Feb 13, 2018

View reviewed changes

stephaniehobson approved these changes Feb 13, 2018

View reviewed changes

Fix bug 1434558, swap search and toolbox components

1613744

jwhitlock merged commit 821fabe into mdn:master Feb 14, 2018

Fix bug 1434558, swap search and toolbox components #4656

Fix bug 1434558, swap search and toolbox components #4656

Uh oh!

Conversation

schalkneethling commented Feb 8, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

To-Do

Uh oh!

jwhitlock commented Feb 9, 2018

Uh oh!

schalkneethling commented Feb 9, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stephaniehobson left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stephaniehobson commented Feb 9, 2018

Uh oh!

schalkneethling commented Feb 12, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov-io commented Feb 12, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

schalkneethling commented Feb 12, 2018

Uh oh!

schalkneethling commented Feb 12, 2018

Uh oh!

jwhitlock commented Feb 12, 2018

Uh oh!

jwhitlock left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

schalkneethling commented Feb 13, 2018

Uh oh!

jwhitlock commented Feb 13, 2018

Uh oh!

jwhitlock commented Feb 13, 2018

Uh oh!

jwhitlock commented Feb 13, 2018

Uh oh!

schalkneethling commented Feb 13, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

stephaniehobson commented Feb 13, 2018

Uh oh!

schalkneethling commented Feb 14, 2018

Uh oh!

jwhitlock commented Feb 14, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

schalkneethling commented Feb 8, 2018 •

edited

Loading

schalkneethling commented Feb 12, 2018 •

edited

Loading

codecov-io commented Feb 12, 2018 •

edited

Loading

schalkneethling commented Feb 13, 2018 •

edited

Loading