Ship review feedback

duckduckgo · Jan 17, 2025 · 2a2eced · 2a2eced
1 parent 9224242
commit 2a2eced
Show file tree

Hide file tree

Showing 18 changed files with 153 additions and 60 deletions.
diff --git a/special-pages/pages/new-tab/app/components/App.js b/special-pages/pages/new-tab/app/components/App.js
@@ -1,10 +1,15 @@
 import { Fragment, h } from 'preact';
 import cn from 'classnames';
 import styles from './App.module.css';
-import { useCustomizerDrawerSettings, usePlatformName } from '../settings.provider.js';
+import { useCustomizerDrawerSettings, useCustomizerKind, usePlatformName } from '../settings.provider.js';
 import { WidgetList } from '../widget-list/WidgetList.js';
 import { useGlobalDropzone } from '../dropzone.js';
-import { Customizer, CustomizerButton, CustomizerMenuPositionedFixed, useContextMenu } from '../customizer/components/Customizer.js';
+import {
+    CustomizerMenu,
+    CustomizerButton,
+    CustomizerMenuPositionedFixed,
+    useContextMenu,
+} from '../customizer/components/CustomizerMenu.js';
 import { useDrawer, useDrawerControls } from './Drawer.js';
 import { CustomizerDrawer } from '../customizer/components/CustomizerDrawer.js';
 import { BackgroundConsumer } from './BackgroundProvider.js';
@@ -23,7 +28,7 @@ export function App() {
     const platformName = usePlatformName();
     const customizerDrawer = useCustomizerDrawerSettings();
 
-    const customizerKind = customizerDrawer.state === 'enabled' ? 'drawer' : 'menu';
+    const customizerKind = useCustomizerKind();
 
     useGlobalDropzone();
     useContextMenu();
@@ -41,6 +46,7 @@ export function App() {
     } = useDrawer(customizerDrawer.autoOpen ? 'visible' : 'hidden');
 
     const tabIndex = useComputed(() => (hidden.value ? -1 : 0));
+    const isOpen = useComputed(() => hidden.value === false);
     const { toggle } = useDrawerControls();
     const { main, browser } = useContext(CustomizerThemesContext);
 
@@ -54,19 +60,21 @@ export function App() {
                             <WidgetList />
                         </div>
                     </div>
-                    <CustomizerMenuPositionedFixed>
-                        {customizerKind === 'menu' && <Customizer />}
-                        {customizerKind === 'drawer' && (
-                            <CustomizerButton
-                                buttonId={buttonId}
-                                menuId={drawerId}
-                                toggleMenu={toggle}
-                                buttonRef={buttonRef}
-                                isOpen={false}
-                            />
-                        )}
-                    </CustomizerMenuPositionedFixed>
                 </main>
+                <CustomizerMenuPositionedFixed>
+                    {customizerKind === 'menu' && <CustomizerMenu />}
+                    {customizerKind === 'drawer' && (
+                        <CustomizerButton
+                            buttonId={buttonId}
+                            menuId={drawerId}
+                            toggleMenu={toggle}
+                            buttonRef={buttonRef}
+                            isOpen={isOpen}
+                            kind={'drawer'}
+                            theme={main}
+                        />
+                    )}
+                </CustomizerMenuPositionedFixed>
                 {customizerKind === 'drawer' && (
                     <aside
                         class={cn(styles.aside, styles.asideLayout, styles.asideScroller)}

diff --git a/special-pages/pages/new-tab/app/components/App.module.css b/special-pages/pages/new-tab/app/components/App.module.css
@@ -52,11 +52,14 @@ body[data-animate-background="true"] {
     color: var(--ntp-text-normal);
 }
 
+
+
 .mainLayout {
-    padding-right: 0;
-    transition: padding-right .3s;
+    will-change: transform;
+    transition: transform .3s;
+
     [data-drawer-visibility='visible'] & {
-        padding-right: var(--ntp-combined-width);
+        transform: translateX(calc(0px - var(--ntp-combined-width) / 2));
     }
 }
 
@@ -110,6 +113,7 @@ body[data-animate-background="true"] {
     right: 0;
     top: 0;
     z-index: 1;
+    will-change: transform;
     transform: translateX(100%);
     transition: transform .3s;
 

diff --git a/special-pages/pages/new-tab/app/customizer/components/BackgroundSection.js b/special-pages/pages/new-tab/app/customizer/components/BackgroundSection.js
@@ -93,6 +93,7 @@ function DefaultPanel({ checked, onClick }) {
                 aria-labelledby={id}
                 role="radio"
                 onClick={onClick}
+                tabindex={checked ? -1 : 0}
             >
                 {checked && <CircleCheck />}
             </button>
@@ -117,6 +118,7 @@ function ColorPanel(props) {
                 data-color-mode={props.color.colorScheme}
                 onClick={props.onClick}
                 aria-checked={props.checked}
+                tabindex={props.checked ? -1 : 0}
                 aria-labelledby={id}
                 role="radio"
                 style={{ background: props.color.hex }}
@@ -144,6 +146,7 @@ function GradientPanel(props) {
                 class={cn(styles.bgPanel, styles.dynamicIconColor)}
                 data-color-mode={props.gradient.colorScheme}
                 aria-checked={props.checked}
+                tabindex={props.checked ? -1 : 0}
                 aria-labelledby={id}
                 style={{
                     background: `url(${props.gradient.path})`,

diff --git a/special-pages/pages/new-tab/app/customizer/components/BrowserThemeSection.js b/special-pages/pages/new-tab/app/customizer/components/BrowserThemeSection.js
@@ -26,7 +26,7 @@ export function BrowserThemeSection(props) {
                     role="radio"
                     type="button"
                     aria-checked={current.value === 'light'}
-                    tabindex={0}
+                    tabindex={current.value === 'light' ? -1 : 0}
                     onClick={() => props.setTheme({ theme: 'light' })}
                 >
                     <span class="sr-only">{t('customizer_browser_theme_label', { type: 'light' })}</span>
@@ -39,7 +39,7 @@ export function BrowserThemeSection(props) {
                     role="radio"
                     type="button"
                     aria-checked={current.value === 'dark'}
-                    tabindex={0}
+                    tabindex={current.value === 'dark' ? -1 : 0}
                     onClick={() => props.setTheme({ theme: 'dark' })}
                 >
                     <span class="sr-only">{t('customizer_browser_theme_label', { type: 'dark' })}</span>
@@ -52,7 +52,7 @@ export function BrowserThemeSection(props) {
                     role="radio"
                     type="button"
                     aria-checked={current.value === 'system'}
-                    tabindex={0}
+                    tabindex={current.value === 'system' ? -1 : 0}
                     onClick={() => props.setTheme({ theme: 'system' })}
                 >
                     <span class="sr-only">{t('customizer_browser_theme_label', { type: 'system' })}</span>

diff --git a/special-pages/pages/new-tab/app/customizer/components/Customizer.examples.js b/special-pages/pages/new-tab/app/customizer/components/Customizer.examples.js
@@ -1,6 +1,6 @@
 import { h } from 'preact';
 import { noop } from '../../utils.js';
-import { CustomizerButton } from './Customizer.js';
+import { CustomizerButton } from './CustomizerMenu.js';
 import { EmbeddedVisibilityMenu, VisibilityMenu } from './VisibilityMenu.js';
 import { BackgroundSection } from './BackgroundSection.js';
 import { ColorSelection } from './ColorSelection.js';
@@ -66,7 +66,7 @@ export const customizerExamples = {
     'customizer-menu': {
         factory: () => (
             <MaxContent>
-                <CustomizerButton isOpen={true} />
+                <CustomizerButton isOpen={true} kind="menu" />
                 <br />
                 <VisibilityMenu
                     rows={[

diff --git a/special-pages/pages/new-tab/app/customizer/components/Customizer.module.css b/special-pages/pages/new-tab/app/customizer/components/Customizer.module.css
@@ -3,7 +3,7 @@
 }
 
 .lowerRightFixed {
-    position: fixed;
+    position: absolute;
     bottom: 1rem;
     right: 1rem;
 }
@@ -56,6 +56,16 @@
             border-color: var(--color-white-at-50);
         }
     }
+
+    &[data-kind="drawer"][aria-expanded="true"] {
+        visibility: hidden;
+    }
+
+    @media screen and (max-width: 800px) {
+        span {
+            display: none;
+        }
+    }
 }
 
 

diff --git a/special-pages/pages/new-tab/app/customizer/components/CustomizerDrawerInner.module.css b/special-pages/pages/new-tab/app/customizer/components/CustomizerDrawerInner.module.css
@@ -63,7 +63,7 @@
         opacity: .8;
     }
     &:focus-visible {
-        outline: 1px solid var(--ntp-focus-outline-color)
+        box-shadow: var(--focus-ring);
     }
 }
 .section {

diff --git a/...b/app/customizer/components/Customizer.js → ...p/customizer/components/CustomizerMenu.js b/...b/app/customizer/components/Customizer.js → ...p/customizer/components/CustomizerMenu.js
@@ -13,7 +13,7 @@ import { VisibilityMenu, VisibilityMenuPopover } from './VisibilityMenu.js';
 /**
  * Represents the NTP customizer. For now it's just the ability to toggle sections.
  */
-export function Customizer() {
+export function CustomizerMenu() {
     const { setIsOpen, buttonRef, dropdownRef, isOpen } = useDropdown();
     const [rowData, setRowData] = useState(/** @type {VisibilityRowData[]} */ ([]));
 
@@ -32,9 +32,9 @@ export function Customizer() {
         function handler() {
             setRowData(getItems());
         }
-        window.addEventListener(Customizer.UPDATE_EVENT, handler);
+        window.addEventListener(CustomizerMenu.UPDATE_EVENT, handler);
         return () => {
-            window.removeEventListener(Customizer.UPDATE_EVENT, handler);
+            window.removeEventListener(CustomizerMenu.UPDATE_EVENT, handler);
         };
     }, [isOpen]);
 
@@ -43,7 +43,14 @@ export function Customizer() {
 
     return (
         <div class={styles.root} ref={dropdownRef}>
-            <CustomizerButton buttonId={BUTTON_ID} menuId={MENU_ID} toggleMenu={toggleMenu} buttonRef={buttonRef} isOpen={isOpen} />
+            <CustomizerButton
+                buttonId={BUTTON_ID}
+                menuId={MENU_ID}
+                toggleMenu={toggleMenu}
+                buttonRef={buttonRef}
+                isOpen={isOpen}
+                kind={'menu'}
+            />
             <div id={MENU_ID} class={cn(styles.dropdownMenu, { [styles.show]: isOpen })} aria-labelledby={BUTTON_ID}>
                 <VisibilityMenuPopover>
                     <VisibilityMenu rows={rowData} />
@@ -53,8 +60,8 @@ export function Customizer() {
     );
 }
 
-Customizer.OPEN_EVENT = 'ntp-customizer-open';
-Customizer.UPDATE_EVENT = 'ntp-customizer-update';
+CustomizerMenu.OPEN_EVENT = 'ntp-customizer-open';
+CustomizerMenu.UPDATE_EVENT = 'ntp-customizer-update';
 
 export function getItems() {
     /** @type {VisibilityRowData[]} */
@@ -64,7 +71,7 @@ export function getItems() {
             next.push(incoming);
         },
     };
-    const event = new CustomEvent(Customizer.OPEN_EVENT, { detail });
+    const event = new CustomEvent(CustomizerMenu.OPEN_EVENT, { detail });
     window.dispatchEvent(event);
     next.sort((a, b) => a.index - b.index);
     return next;
@@ -102,20 +109,24 @@ export function useContextMenu() {
  * @param {object} props
  * @param {string} [props.menuId]
  * @param {string} [props.buttonId]
- * @param {boolean} props.isOpen
+ * @param {import("@preact/signals").Signal<boolean>|boolean} props.isOpen
  * @param {() => void} [props.toggleMenu]
  * @param {import("preact").Ref<HTMLButtonElement>} [props.buttonRef]
+ * @param {import('@preact/signals').Signal<'light' | 'dark'>} [props.theme]
+ * @param {"menu" | "drawer"} props.kind
  */
-export function CustomizerButton({ menuId, buttonId, isOpen, toggleMenu, buttonRef }) {
+export function CustomizerButton({ menuId, buttonId, isOpen, toggleMenu, buttonRef, kind, theme }) {
     const { t } = useTypedTranslation();
     return (
         <button
             ref={buttonRef}
-            className={styles.customizeButton}
+            class={styles.customizeButton}
             onClick={toggleMenu}
             aria-haspopup="true"
             aria-expanded={isOpen}
             aria-controls={menuId}
+            data-kind={kind}
+            data-theme={theme}
             id={buttonId}
         >
             <CustomizeIcon />
@@ -210,11 +221,11 @@ export function useCustomizer({ title, id, icon, toggle, visibility, index }) {
         const handler = (/** @type {CustomEvent<any>} */ e) => {
             e.detail.register({ title, id, icon, toggle, visibility, index });
         };
-        window.addEventListener(Customizer.OPEN_EVENT, handler);
-        return () => window.removeEventListener(Customizer.OPEN_EVENT, handler);
+        window.addEventListener(CustomizerMenu.OPEN_EVENT, handler);
+        return () => window.removeEventListener(CustomizerMenu.OPEN_EVENT, handler);
     }, [title, id, icon, toggle, visibility, index]);
 
     useEffect(() => {
-        window.dispatchEvent(new Event(Customizer.UPDATE_EVENT));
+        window.dispatchEvent(new Event(CustomizerMenu.UPDATE_EVENT));
     }, [visibility]);
 }
diff --git a/special-pages/pages/new-tab/app/customizer/components/VisibilityMenu.js b/special-pages/pages/new-tab/app/customizer/components/VisibilityMenu.js
@@ -11,7 +11,7 @@ import { CustomizerThemesContext } from '../CustomizerProvider.js';
 
 /**
  * @import { Widgets, WidgetConfigItem } from '../../../types/new-tab.js'
- * @import { VisibilityRowData, VisibilityRowState } from './Customizer.js'
+ * @import { VisibilityRowData, VisibilityRowState } from './CustomizerMenu.js'
  */
 
 /**

diff --git a/special-pages/pages/new-tab/app/customizer/components/VisibilityMenu.module.css b/special-pages/pages/new-tab/app/customizer/components/VisibilityMenu.module.css
@@ -24,7 +24,7 @@
 }
 
 .embedded {
-    font-size: var(--small-label-font-size);
+    font-size: var(--body-font-size);
     gap: 12px;
 }
 
@@ -34,22 +34,21 @@
     gap: 10px;
     white-space: nowrap;
     height: calc(28 * var(--px-in-rem));
-
-    > * {
-        min-width: 0;
-    }
-
-    label {
-        margin-left: auto;
-    }
 }
 
 .menuItemLabelEmbedded {
     white-space: normal;
     gap: 6px;
     height: auto;
-    > * {
-        min-width: auto;
+
+    /* push the input over to the RHS */
+    *:last-child {
+        margin-left: auto;
+    }
+
+    /* Apply a theme-specific focus ring on the switch **/
+    input:focus-visible + * {
+        box-shadow: var(--focus-ring)
     }
 }
 

diff --git a/special-pages/pages/new-tab/app/customizer/components/VisibilityMenuSection.js b/special-pages/pages/new-tab/app/customizer/components/VisibilityMenuSection.js
@@ -1,21 +1,21 @@
 import { useLayoutEffect, useState } from 'preact/hooks';
-import { Customizer, getItems } from './Customizer.js';
+import { CustomizerMenu, getItems } from './CustomizerMenu.js';
 import { EmbeddedVisibilityMenu } from './VisibilityMenu.js';
 import { h } from 'preact';
 
 export function VisibilityMenuSection() {
     const [rowData, setRowData] = useState(() => {
-        const items = /** @type {import("./Customizer.js").VisibilityRowData[]} */ (getItems());
+        const items = /** @type {import("./CustomizerMenu.js").VisibilityRowData[]} */ (getItems());
         return items;
     });
     useLayoutEffect(() => {
         function handler() {
             setRowData(getItems());
         }
 
-        window.addEventListener(Customizer.UPDATE_EVENT, handler);
+        window.addEventListener(CustomizerMenu.UPDATE_EVENT, handler);
         return () => {
-            window.removeEventListener(Customizer.UPDATE_EVENT, handler);
+            window.removeEventListener(CustomizerMenu.UPDATE_EVENT, handler);
         };
     }, []);
 

diff --git a/special-pages/pages/new-tab/app/customizer/customizer.md b/special-pages/pages/new-tab/app/customizer/customizer.md
@@ -139,7 +139,7 @@ title: Customizer
         "background": { "kind": "color", "value": "color01" }
       } 
       ```
-
+  
 - {@link "NewTab Messages".CustomizerSetThemeNotification `customizer_setTheme`}.
     - Sends {@link "NewTab Messages".CustomizerSetBackgroundNotify} whenever needed.
     - For example:
@@ -151,8 +151,7 @@ title: Customizer
 
 - {@link "NewTab Messages".CustomizerUploadNotification `customizer_upload`}.
     - Sent to trigger a file upload
-
-
+
 - {@link "NewTab Messages".CustomizerDeleteImageNotification `customizer_deleteImage`}.
     - Sends {@link "NewTab Messages".CustomizerDeleteImageNotify} whenever needed.
     - For example:
-Original file line number
+Diff line change
@@ Expand Up @@
     /**
      * @import { Widgets, WidgetConfigItem } from '../../../types/new-tab.js'
-     * @import { VisibilityRowData, VisibilityRowState } from './Customizer.js'
+     * @import { VisibilityRowData, VisibilityRowState } from './CustomizerMenu.js'
      */
     /**
@@ Expand Down @@