The mobile-menu Sass partial includes both the mobile-nav and nav--mobile components. Also, the mobile-nav component doesn’t match the name of the Sass partial.

CommentFileSizeAuthor
#6 interdiff.txt1.17 KBdcmouyard
#6 gesso-mobile-menu-2453173-6.patch38.64 KBdcmouyard
#2 gesso-mobile-menu-2453173-2.patch40.1 KBdcmouyard
#1 gesso-mobile-menu-2453173-1.patch40.88 KBdcmouyard

Comments

dcmouyard’s picture

dcmouyard commented
Issue summary: View changes
Status: Active » Needs review
StatusFileSize
new gesso-mobile-menu-2453173-1.patch40.88 KB

This patch renames the mobile-nav component to mobile-menu in mobile-menu.js and moves the nav--mobile styles into its own Sass partial.

dcmouyard’s picture

dcmouyard commented
StatusFileSize
new gesso-mobile-menu-2453173-2.patch40.1 KB

Re-rolled patch so that it applies cleanly to 7.x-1.x.

adamjuran’s picture

adamjuran commented

Patch applies cleanly. Any particular reason the class/partial for mobile menu "nav--mobile" and the one for main menu "nav--main-menu"? What are your thoughts on naming consistency, Dan?

dcmouyard’s picture

dcmouyard commented

The naming convention is just nav--variant-name. For most menus, the variant name will be the machine name of the menu (e.g., main-menu, primary-local-tasks, menu-custom, etc.) We have full control of the variant name for the mobile menu in mobile-menu.js, though, so it could be anything. Would you prefer nav--mobile-menu instead of nav--mobile?

adamjuran’s picture

adamjuran commented

I think I would prefer nav--mobile-menu, yes.

dcmouyard’s picture

dcmouyard commented
StatusFileSize
new gesso-mobile-menu-2453173-6.patch38.64 KB
new interdiff.txt1.17 KB
adamjuran’s picture

adamjuran commented
Status: Needs review » Reviewed & tested by the community

Looks good. RTBC.

  • dcmouyard committed dc11a2c on 7.x-1.x 
    Issue #2453173 by dcmouyard, scaragucc': Refactored mobile menu styles...
dcmouyard’s picture

dcmouyard commented
Status: Reviewed & tested by the community » Fixed

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.