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

shotaK's Add context to the menu collapsible factory target elements #1382

Merged
merged 1 commit into from Nov 29, 2018

Conversation

Projects
None yet
5 participants
@Ubersmake
Copy link
Contributor

Ubersmake commented Nov 14, 2018

Rebased version of #1299 by shotaK.

What?

I've just duplicated a menu in global.js and in templates and supplied a proper context:

global.js

menu('[data-menu]');
menu('[data-menumobile]');

It seemed to work fine, except when I was clicking the first menu items, the second menu items were opening. After some investigation, I've found out that the context was not specified for the targeted menu items. I've added the context to them and it worked fine.

Testing

This has no effect on Cornerstone as it is, but benefits anyone extending Cornerstone.

To test, make the following changes:

Change /assets/js/theme/global.js so that menu() is defined twice with two different contexts:

@@ -31,7 +31,8 @@ export default class Global extends PageManager {
         cartPreview();
         compareProducts(this.context.urls);
         carousel();
-        menu();
+        menu('[data-menu]');
+        menu('[data-menu-duplicate]');
         mobileMenuToggle();
         privacyCookieNotification();
         maintenanceMode(this.context.maintenanceMode);

Add something to /templates/components/common/header.html so that the second menu can be displayed:

@@ -28,4 +28,12 @@
     <div class="navPages-container" id="menu" data-menu>
         {{> components/common/navigation-menu}}
     </div>
+
+    <div style="height:100px">
+        <p>Some Spacing</p>
+    </div>
+
+    <div class="navPages-container" id="menu" data-menu-duplicate>
+        {{> components/common/navigation-menu-2}}
+    </div>
 </header>

Add a new file for that new template being referenced, templates/components/common/navigation-menu-2.html. Note the something-else, an attribute which is being used as a "tell" to verify that things are working as expected.

<nav class="navPages">
    <ul class="navPages-list{{#if theme_settings.navigation_design '!==' 'simple'}} navPages-list-depth-max{{/if}}">
        {{#each categories}}
            <li class="navPages-item" something-else>
                {{> components/common/navigation-list}}
            </li>
        {{/each}}    
    </ul>
</nav>
With fix:

shotakwith

Notice how there are two menu bars, and the "duplicate" is correctly tagged with the something-else attribute.

Without fix:

shotakwithout

Only the first menu can be accessed here. something-else does not appear, and only the first menu is opened and interacted with while inspecting the DOM.

And for good measure, Cornerstone 2.6.0 with the fix:

cornerstonewith

@bigbot

This comment has been minimized.

Copy link

bigbot commented Nov 14, 2018

@Ubersmake Ubersmake force-pushed the Ubersmake:shotak-collapsible-context branch from 462c285 to d54cd0d Nov 14, 2018

@Ubersmake Ubersmake force-pushed the Ubersmake:shotak-collapsible-context branch from d54cd0d to 6875edd Nov 29, 2018

@Ubersmake Ubersmake merged commit c4f5fbe into bigcommerce:master Nov 29, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@Ubersmake Ubersmake deleted the Ubersmake:shotak-collapsible-context branch Nov 29, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment