Merge pull request #1336 from NoRedInk/tessa/adjust-sidenav-hamburger

Adjust sidenav mobile behavior

component-catalog-app/App.elm

@@ -127,7 +127,11 @@ update action model =
                 route =
                     Routes.fromLocation model.moduleStates location
             in
-            ( { model | route = route, previousRoute = Just model.route }
+            ( { model
+                | route = route
+                , previousRoute = Just model.route
+                , isSideNavOpen = False
+              }
             , Maybe.map FocusOn (Routes.headerId route)
                 |> Maybe.withDefault None
             )

src/Nri/Ui/MediaQuery/V1.elm

@@ -15,6 +15,7 @@ module Nri.Ui.MediaQuery.V1 exposing
   - remove min-width:1 from media queries in order to support better composibility
   - adds narrowMobileBreakpoint and deprecates narrowMobileBreakPoint
   - adds withViewport for convenience when matching specific viewport size ranges
+  - fix `not` queries to not overlap with the regular breakpoint queries
 
 Standard media queries for responsive pages.
 
@@ -112,7 +113,7 @@ mobile =
 -}
 notMobile : MediaQuery
 notMobile =
-    only screen [ minWidth mobileBreakpoint ]
+    Css.Media.not screen [ maxWidth mobileBreakpoint ]
 
 
 {-| 1000px
@@ -133,7 +134,7 @@ quizEngineMobile =
 -}
 notQuizEngineMobile : MediaQuery
 notQuizEngineMobile =
-    only screen [ minWidth quizEngineBreakpoint ]
+    Css.Media.not screen [ maxWidth quizEngineBreakpoint ]
 
 
 {-| 750px
@@ -154,7 +155,7 @@ narrowMobile =
 -}
 notNarrowMobile : MediaQuery
 notNarrowMobile =
-    only screen [ minWidth narrowMobileBreakpoint ]
+    Css.Media.not screen [ maxWidth narrowMobileBreakpoint ]
 
 
 {-| 500px

src/Nri/Ui/SideNav/V4.elm

@@ -19,6 +19,7 @@ module Nri.Ui.SideNav.V4 exposing
 
   - add missing aria-current=page attribute
   - don't render an empty nav when there are no entries
+  - adjust closed sidenav toggle button styles
 
 
 ### Changes from V3
@@ -64,6 +65,7 @@ import Css.Media
 import Html.Styled
 import Html.Styled.Attributes as Attributes
 import Html.Styled.Events as Events
+import Maybe.Extra
 import Nri.Ui
 import Nri.Ui.AnimatedIcon.V1 as AnimatedIcon
 import Nri.Ui.ClickableSvg.V2 as ClickableSvg
@@ -246,12 +248,17 @@ view config navAttributes entries =
                 , marginRight Css.zero
                 , marginBottom (Css.px 20)
                 , width (pct 100)
+                , case Maybe.map .isOpen appliedNavAttributes.collapsible of
+                    Just _ ->
+                        Css.padding (Css.px 10)
+
+                    Nothing ->
+                        Css.batch []
                 ]
             ]
     in
     div [ Attributes.css (defaultCss ++ appliedNavAttributes.css) ]
         [ viewSkipLink config.onSkipNav
-        , viewJust (viewOpenCloseButton sidenavId appliedNavAttributes.navLabel) appliedNavAttributes.collapsible
         , case entries of
             [] ->
                 text ""
@@ -266,8 +273,8 @@ defaultSideNavId =
     "sidenav"
 
 
-viewOpenCloseButton : String -> Maybe String -> CollapsibleConfig msg -> Html msg
-viewOpenCloseButton sidenavId navLabel_ { isOpen, toggle, isTooltipOpen, toggleTooltip } =
+viewOpenCloseButton : String -> Maybe String -> Maybe String -> CollapsibleConfig msg -> Html msg
+viewOpenCloseButton sidenavId navLabel_ currentEntry { isOpen, toggle, isTooltipOpen, toggleTooltip } =
     let
         name =
             Maybe.withDefault "sidebar" navLabel_
@@ -284,64 +291,105 @@ viewOpenCloseButton sidenavId navLabel_ { isOpen, toggle, isTooltipOpen, toggleT
                     |> Svg.withCss [ Css.transform (rotate (deg 180)) ]
                 )
 
-        trigger tooltipAttributes =
+        trigger attributes =
             ClickableSvg.button action
                 icon_
-                [ ClickableSvg.custom
+                ([ ClickableSvg.custom
                     [ Aria.controls [ sidenavId ]
                     , Aria.expanded isOpen
                     ]
-                , ClickableSvg.custom tooltipAttributes
-                , ClickableSvg.onClick (toggle (not isOpen))
-                , ClickableSvg.secondary
-                , ClickableSvg.withBorder
-                , ClickableSvg.iconForMobile (AnimatedIcon.mobileOpenClose isOpen)
-                ]
-    in
-    Tooltip.view
-        { trigger = trigger
-        , id = "open-close-sidebar-tooltip"
-        }
-        [ Tooltip.open isTooltipOpen
-        , Tooltip.onToggle toggleTooltip
-        , Tooltip.plaintext action
-        , Tooltip.smallPadding
-        , Tooltip.fitToContent
-        , if isOpen then
-            Tooltip.onLeft
+                 , ClickableSvg.onClick (toggle (not isOpen))
+                 , ClickableSvg.secondary
+                 , ClickableSvg.withBorder
+                 , ClickableSvg.iconForMobile (AnimatedIcon.mobileOpenClose isOpen)
+                 ]
+                    ++ attributes
+                )
 
-          else
-            Tooltip.onRight
-        , Tooltip.onRightForMobile
-        , Tooltip.containerCss
-            (if isOpen then
-                [ Css.Media.withMedia [ MediaQuery.notMobile ]
-                    [ Css.position Css.absolute
-                    , Css.top (Css.px 10)
-                    , Css.right (Css.px 10)
+        nonMobileTooltipView =
+            Tooltip.view
+                { trigger = \tooltipAttributes -> trigger [ ClickableSvg.custom tooltipAttributes ]
+                , id = "open-close-sidebar-tooltip"
+                }
+                [ Tooltip.open isTooltipOpen
+                , Tooltip.onToggle toggleTooltip
+                , Tooltip.plaintext action
+                , Tooltip.smallPadding
+                , Tooltip.fitToContent
+                , if isOpen then
+                    Tooltip.onLeft
+
+                  else
+                    Tooltip.onRight
+                , Tooltip.containerCss
+                    [ -- Hide the tooltip for mobile. We'll display static text instead
+                      Css.Media.withMedia [ MediaQuery.mobile ]
+                        [ Css.display Css.none ]
+                    ]
+                , Tooltip.containerCss
+                    (if isOpen then
+                        [ Css.Media.withMedia [ MediaQuery.notMobile ]
+                            [ Css.position Css.absolute
+                            , Css.top (Css.px 10)
+                            , Css.right (Css.px 10)
+                            ]
+                        ]
+
+                     else
+                        []
+                    )
+                ]
+
+        mobileButtonView =
+            div
+                [ Attributes.css
+                    [ -- Hide the plain button/static text if not on the mobile view
+                      Css.display Css.none
+                    , Css.Media.withMedia [ MediaQuery.mobile ]
+                        [ Css.displayFlex ]
                     ]
                 ]
-
-             else
-                []
-            )
+                [ trigger []
+                , viewJust mobileCurrentPage currentEntry
+                ]
+    in
+    div []
+        [ nonMobileTooltipView
+        , mobileButtonView
         ]
 
 
+mobileCurrentPage : String -> Html msg
+mobileCurrentPage name =
+    span
+        [ AttributesExtra.nriDescription "mobile-current-page-name"
+        , Attributes.css (sharedEntryStyles ++ [ Css.display Css.inline, Css.padding (Css.px 8) ])
+        ]
+        [ text name ]
+
+
 viewNav : String -> Config route msg -> NavAttributeConfig msg -> List (Entry route msg) -> Bool -> Html msg
 viewNav sidenavId config appliedNavAttributes entries showNav =
+    let
+        currentEntry =
+            currentRouteName config.isCurrentRoute entries
+
+        entryStyles =
+            if showNav then
+                []
+
+            else
+                [ Css.display Css.none ]
+    in
     nav
         ([ Maybe.map Aria.label appliedNavAttributes.navLabel
          , Just (Attributes.id sidenavId)
-         , if showNav then
-            Nothing
-
-           else
-            Just (Attributes.css [ Css.display Css.none ])
          ]
             |> List.filterMap identity
         )
-        (List.map (viewSidebarEntry config []) entries)
+        (viewJust (viewOpenCloseButton sidenavId appliedNavAttributes.navLabel currentEntry) appliedNavAttributes.collapsible
+            :: List.map (viewSidebarEntry config entryStyles) entries
+        )
 
 
 viewSkipLink : msg -> Html msg
@@ -385,7 +433,9 @@ viewSidebarEntry config extraStyles entry_ =
                         )
                         []
                         [ text entryConfig.title ]
-                        :: List.map (viewSidebarEntry config [ marginLeft (px 20) ]) children
+                        :: List.map
+                            (viewSidebarEntry config (marginLeft (px 20) :: extraStyles))
+                            children
                     )
 
             else
@@ -415,6 +465,28 @@ anyLinkDescendants f children =
         children
 
 
+currentRouteName : (route -> Bool) -> List (Entry route msg) -> Maybe String
+currentRouteName isCurrentRoute_ entries =
+    List.foldl
+        (\entry_ acc ->
+            Maybe.Extra.or acc
+                (case entry_ of
+                    Entry children_ entryConfig ->
+                        case Maybe.map isCurrentRoute_ entryConfig.route of
+                            Just True ->
+                                Just entryConfig.title
+
+                            _ ->
+                                currentRouteName isCurrentRoute_ children_
+
+                    Html _ ->
+                        acc
+                )
+        )
+        Nothing
+        entries
+
+
 viewSidebarLeaf :
     Config route msg
     -> List Style

tests/Spec/Nri/Ui/SideNav.elm

@@ -4,7 +4,7 @@ import Accessibility.Aria as Aria
 import Expect exposing (Expectation)
 import Html.Attributes as Attributes
 import Html.Styled exposing (toUnstyled)
-import Nri.Ui.SideNav.V4 as SideNav exposing (Entry, NavAttribute)
+import Nri.Ui.SideNav.V4 as SideNav exposing (Entry)
 import Test exposing (..)
 import Test.Html.Query as Query
 import Test.Html.Selector exposing (..)
@@ -22,29 +22,29 @@ currentPageTests =
     [ test "without entries" <|
         \() ->
             []
-                |> viewQuery { currentRoute = "cactus" } []
+                |> viewQuery { currentRoute = "cactus" }
                 |> Query.hasNot [ tag "nav" ]
     , test "with 1 partial entry" <|
         \() ->
             [ SideNav.entry "Cactus" [] ]
-                |> viewQuery { currentRoute = "a-different-route" } []
-                |> Query.hasNot [ attribute Aria.currentPage ]
+                |> viewQuery { currentRoute = "a-different-route" }
+                |> expectNoCurrentPage
     , test "with 1 complete but not current entry" <|
         \() ->
             [ SideNav.entry "Cactus" [ SideNav.href "cactus" ] ]
-                |> viewQuery { currentRoute = "a-different-route" } []
-                |> Query.hasNot [ attribute Aria.currentPage ]
+                |> viewQuery { currentRoute = "a-different-route" }
+                |> expectNoCurrentPage
     , test "with 1 current entry" <|
         \() ->
             [ SideNav.entry "Cactus" [ SideNav.href "cactus" ] ]
-                |> viewQuery { currentRoute = "cactus" } []
+                |> viewQuery { currentRoute = "cactus" }
                 |> expectCurrentPage "Cactus" "cactus"
     , test "with multiple entries, one of which is current" <|
         \() ->
             [ SideNav.entry "Cactus" [ SideNav.href "cactus" ]
             , SideNav.entry "Epiphyllum" [ SideNav.href "epiphyllum" ]
             ]
-                |> viewQuery { currentRoute = "cactus" } []
+                |> viewQuery { currentRoute = "cactus" }
                 |> expectCurrentPage "Cactus" "cactus"
     , test "with a currently-selected entry with children" <|
         \() ->
@@ -52,7 +52,7 @@ currentPageTests =
                 [ SideNav.href "cactus" ]
                 [ SideNav.entry "Epiphyllum" [ SideNav.href "epiphyllum" ] ]
             ]
-                |> viewQuery { currentRoute = "cactus" } []
+                |> viewQuery { currentRoute = "cactus" }
                 |> expectCurrentPage "Cactus" "cactus"
     , test "with a currently-selected child entry" <|
         \() ->
@@ -60,17 +60,30 @@ currentPageTests =
                 [ SideNav.href "cactus" ]
                 [ SideNav.entry "Epiphyllum" [ SideNav.href "epiphyllum" ] ]
             ]
-                |> viewQuery { currentRoute = "epiphyllum" } []
+                |> viewQuery { currentRoute = "epiphyllum" }
                 |> expectCurrentPage "Epiphyllum" "epiphyllum"
     ]
 
 
+expectNoCurrentPage : Query.Single msg -> Expectation
+expectNoCurrentPage =
+    Expect.all
+        [ Query.hasNot [ attribute Aria.currentPage ]
+        , Query.hasNot [ mobilePageName ]
+        ]
+
+
 expectCurrentPage : String -> String -> Query.Single msg -> Expectation
 expectCurrentPage name href_ =
     Expect.all
         [ Query.findAll [ attribute Aria.currentPage ]
             >> Query.count (Expect.equal 1)
         , Query.has [ currentPage name href_ ]
+        , -- for mobile, shows the currently-selected route in text
+          Query.has
+            [ mobilePageName
+            , containing [ text name ]
+            ]
         ]
 
 
@@ -84,18 +97,28 @@ currentPage name href_ =
         ]
 
 
+mobilePageName : Selector
+mobilePageName =
+    attribute (Attributes.attribute "data-nri-description" "mobile-current-page-name")
+
+
 viewQuery :
     { currentRoute : String }
-    -> List (NavAttribute ())
     -> List (Entry String ())
     -> Query.Single ()
-viewQuery { currentRoute } navAttributes entries =
+viewQuery { currentRoute } entries =
     SideNav.view
         { isCurrentRoute = (==) currentRoute
         , routeToString = identity
         , onSkipNav = ()
         }
-        navAttributes
+        [ SideNav.collapsible
+            { isOpen = True
+            , toggle = \_ -> ()
+            , isTooltipOpen = False
+            , toggleTooltip = \_ -> ()
+            }
+        ]
         entries
         |> toUnstyled
         |> Query.fromHtml