addresses PR feedback for navigation

brave · Nov 11, 2023 · ef18c43 · ef18c43
1 parent 6dfb887
commit ef18c43
Show file tree

Hide file tree

Showing 6 changed files with 139 additions and 55 deletions.
diff --git a/src/components/navigation/navigation.stories.svelte b/src/components/navigation/navigation.stories.svelte
@@ -7,6 +7,7 @@
   import NavigationItem from './navigationItem.svelte'
   import NavigationHeader from './navigationHeader.svelte'
   import NavigationActions from './navigationActions.svelte'
+  import NavigationMenu from './navigationMenu.svelte'
   import Icon from '../icon/icon.svelte'
 </script>
 
@@ -24,28 +25,32 @@
       <h1><span class="logo-mark">Brave</span> Accounts</h1>
     </NavigationHeader>
 
-    {#each [1, 2, 3, 4, 5] as item}
-      <NavigationItem href="#" icon="browser-ntp-widget" isActive={item === 3}
-        >Item {item}</NavigationItem
-      >
-    {/each}
-    <NavigationItem href="#" icon="browser-ntp-widget">
-      Item with sub nav
-      <ul slot="subnav">
-        <NavigationItem href="#" icon="agenda" isActive={true}
-          >Testing</NavigationItem
+    <NavigationMenu>
+      {#each [1, 2, 3, 4, 5] as item}
+        <NavigationItem
+          href="#"
+          icon="browser-ntp-widget"
+          isCurrent={item === 3}>Item {item}</NavigationItem
         >
+      {/each}
+      <NavigationItem href="#" icon="browser-ntp-widget">
+        Item with sub nav
+        <NavigationMenu slot="subnav">
+          <NavigationItem href="#" icon="agenda" isCurrent={true}
+            >Testing</NavigationItem
+          >
 
-        <NavigationItem href="#" icon="browser-ntp-widget">
-          Item with sub nav
-          <ul slot="subnav">
-            <NavigationItem href="#" icon="agenda" isActive={true}
-              >Testing</NavigationItem
-            >
-          </ul>
-        </NavigationItem>
-      </ul>
-    </NavigationItem>
+          <NavigationItem href="#" icon="browser-ntp-widget">
+            Item with sub nav
+            <NavigationMenu slot="subnav">
+              <NavigationItem href="#" icon="agenda" isCurrent={true}
+                >Testing</NavigationItem
+              >
+            </NavigationMenu>
+          </NavigationItem>
+        </NavigationMenu>
+      </NavigationItem>
+    </NavigationMenu>
   </Navigation>
 </Template>
 

diff --git a/src/components/navigation/navigation.svelte b/src/components/navigation/navigation.svelte
@@ -2,7 +2,7 @@
   export let kind: 'vertical' | 'horizontal' = 'vertical'
 </script>
 
-<nav
+<div
   class="leo-navigation"
   class:isVertical={kind === 'vertical'}
   class:isHorizontal={kind === 'horizontal'}
@@ -11,29 +11,21 @@
     <slot name="header" />
   {/if}
 
-  <ul>
+  <nav>
     <slot />
-  </ul>
+  </nav>
 
   {#if $$slots.actions}
     <slot name="actions" />
   {/if}
-</nav>
+</div>
 
 <style lang="scss">
   .leo-navigation {
     --nav-direction: row;
     display: flex;
     flex-direction: var(--nav-direction);
-
-    :global(ul) {
-      display: flex;
-      flex-direction: var(--nav-direction);
-      gap: var(--leo-spacing-m);
-      margin: 0;
-      padding: var(--leo-spacing-2xl) 0;
-      list-style: none;
-    }
+    height: 100%;
 
     &.isVertical {
       --nav-direction: column;

diff --git a/src/components/navigation/navigationActions.svelte b/src/components/navigation/navigationActions.svelte
@@ -4,7 +4,8 @@
 
 <style>
   .leo-navigation-actions {
+    border-top: 1px solid var(--leo-color-divider-subtle);
     margin-top: auto;
-    padding: var(--leo-spacing-2xl) var(--leo-spacing-xl);
+    padding: var(--leo-spacing-2xl) 0;
   }
 </style>
diff --git a/src/components/navigation/navigationHeader.svelte b/src/components/navigation/navigationHeader.svelte
@@ -9,6 +9,6 @@
     gap: var(--leo-spacing-m);
     align-items: center;
     justify-self: self-start;
-    padding: var(--leo-spacing-2xl) 20px;
+    padding: var(--leo-navigation-header-padding, var(--leo-spacing-2xl)) 20px;
   }
 </style>
diff --git a/src/components/navigation/navigationItem.svelte b/src/components/navigation/navigationItem.svelte
@@ -1,43 +1,111 @@
 <script lang="ts">
+  import { createEventDispatcher, onMount } from 'svelte'
   import type { SvelteHTMLElements } from 'svelte/elements'
   import type { IconName } from '../../../icons/meta'
   import Icon from '../icon/icon.svelte'
 
+  // This black magic comes from this thread:
+  // https://github.com/sveltejs/language-tools/issues/442#issuecomment-1278618531
+  //
+  // To quote that thread - This is "absolute bonkers!"
+  //
+  // It's interesting, minor variations which I would expect to work on don't,
+  // and this is the only combination which seems to do what we want and I'm not
+  // clear on why. You're welcome to try other approaches here.
+  //
+  // Tips, for if things aren't working right:
+  // 1) npm run gen-types
+  // 2) Reload your VSCode Window (sometimes the Svelte Type Checker struggles).
+  // 3) Make sure any script tags on your component have a `lang="ts"` attribute.
   type Href = $$Generic<string | undefined>
+  type Disabled = $$Generic<undefined extends Href ? boolean : undefined>
+  type ExcludedProps = 'size' | 'href' | 'hreflang'
 
-  type $$Props = Omit<SvelteHTMLElements['a'], 'class' | 'href'> & {
-    href: Href
+  type CommonProps = {
+    inList?: boolean
     icon?: IconName
-    isActive?: boolean
   }
 
-  export let href: Href
+  type ButtonProps = CommonProps &
+    Omit<Partial<SvelteHTMLElements['button']>, ExcludedProps> & {
+      isDisabled?: Disabled
+      isLoading?: boolean
+      href?: never
+    }
+
+  type LinkProps = CommonProps &
+    Omit<Partial<SvelteHTMLElements['a']>, ExcludedProps> & {
+      href: Href
+      isCurrent?: boolean
+    }
+
+  type $$Props = LinkProps | ButtonProps
+
+  export let href: Href = undefined
   export let icon: IconName = undefined
-  export let isActive: boolean = false
+  export let isLoading: boolean = false
+  export let isDisabled: boolean = false
+  export let isCurrent: boolean = window.location.pathname === href
+  export let inList: boolean = true
+
+  const checkIfCurrent = () => {
+    isCurrent = window.location.pathname === href
+  }
+
+  $: tag = href ? 'a' : ('button' as 'a' | 'button')
+
+  /**
+   * Optional click handler
+   */
+  const dispatch = createEventDispatcher()
+  function onClick(event) {
+    dispatch('click', event)
+  }
+
+  onMount(() => {
+    ;['pushState', 'replaceState'].forEach((name) => {
+      const original = history[name]
+      history[name] = function () {
+        original.apply(history, arguments)
+        checkIfCurrent()
+      }
+    })
+
+    window.addEventListener('popstate', checkIfCurrent)
+  })
 </script>
 
-<li class="leo-navigation-item">
-  <a {href} {...$$restProps} class:isActive>
+<svelte:element this={inList ? "li" : "div"} class="leo-navigation-item">
+  <svelte:element
+    this={tag}
+    href={href || undefined}
+    disabled={isLoading || isDisabled || undefined}
+    on:click={onClick}
+    {...$$restProps}
+    class:isCurrent
+  >
     {#if icon}
       <Icon name={icon} />
     {/if}
     <slot />
-  </a>
+  </svelte:element>
 
   {#if $$slots.subnav}
     <slot name="subnav" />
   {/if}
-</li>
+</svelte:element>
 
 <style lang="scss">
   .leo-navigation-item {
-    --color: var(--leo-color-text-secondary);
+    --nav-item-color: var(--leo-color-text-secondary);
     --leo-icon-color: var(--leo-color-icon-default);
     --leo-icon-size: var(--leo-icon-s);
 
-    a {
+    a,
+    button {
       cursor: pointer;
       display: flex;
+      width: 100%;
       gap: var(--leo-spacing-xl);
       align-items: center;
       height: 48px;
@@ -49,7 +117,7 @@
       text-decoration: none;
 
       font: var(--leo-font-components-button-default);
-      color: var(--color);
+      color: var(--nav-item-color);
 
       &:hover {
         background: var(--leo-color-container-highlight);
@@ -59,8 +127,8 @@
         box-shadow: var(--leo-effect-focus-state);
       }
 
-      &.isActive {
-        --color: var(--leo-color-text-interactive);
+      &.isCurrent {
+        --nav-item-color: var(--leo-color-text-interactive);
         --leo-icon-color: var(--leo-color-icon-interactive);
 
         &::before {
@@ -77,11 +145,5 @@
         }
       }
     }
-
-    :global(ul) {
-      padding: var(--leo-spacing-m) 0 0;
-      margin-left: 35px;
-      border-left: 1px solid var(--leo-color-divider-subtle);
-    }
   }
 </style>
diff --git a/src/components/navigation/navigationMenu.svelte b/src/components/navigation/navigationMenu.svelte
@@ -0,0 +1,24 @@
+<script lang="ts">
+  let isSubnav = $$props.slot === "subnav"
+</script>
+
+<ul class:isSubnav>
+  <slot />
+</ul>
+
+<style>
+  ul {
+    display: flex;
+    flex-direction: var(--nav-direction);
+    gap: var(--leo-spacing-m);
+    margin: 0;
+    padding: var(--leo-spacing-2xl) 0;
+    list-style: none;
+  }
+
+  ul.isSubnav {
+    padding: var(--leo-spacing-m) 0 0;
+    margin-left: 33px;
+    border-left: 1px solid var(--leo-color-divider-subtle);
+  }
+</style>