Fixed tiers and scrolling bugs with AdminX (#18138)

refs https://github.com/TryGhost/Product/issues/3832 - Updated tier benefits to be added when pressing enter - Updated tier sorting logic to make sure free comes first - Updated scrolling behaviour and made the sidebar scroll automatically - Added placeholder backdrop when loading a modal to prevent flashing And other minor fixes - Removed placeholder from newsletter footer field - Updated theme installation to work with the default theme
2024-11-28 14:03:48 +03:00 · 2023-09-14 15:09:51 +01:00 · 2023-09-14 15:09:51 +01:00 · a79de45392
commit a79de45392
parent af2459c863
12 changed files with 176 additions and 65 deletions
--- a/apps/admin-x-settings/src/MainContent.tsx
+++ b/apps/admin-x-settings/src/MainContent.tsx
@ -6,6 +6,7 @@ import Users from './components/settings/general/Users';
 import useRouting from './hooks/useRouting';
 import {ReactNode, useEffect} from 'react';
 import {canAccessSettings, isEditorUser} from './api/users';
+import {topLevelBackdropClasses} from './admin-x-ds/global/modal/Modal';
 import {useGlobalData} from './components/providers/GlobalDataProvider';

 const Page: React.FC<{children: ReactNode}> = ({children}) => {
@ -22,7 +23,7 @@ const Page: React.FC<{children: ReactNode}> = ({children}) => {

 const MainContent: React.FC = () => {
    const {currentUser} = useGlobalData();
-    const {route, updateRoute} = useRouting();
+    const {route, updateRoute, loadingModal} = useRouting();

    useEffect(() => {
        if (!canAccessSettings(currentUser) && route !== `users/show/${currentUser.slug}`) {
@ -47,6 +48,8 @@ const MainContent: React.FC = () => {

    return (
        <Page>
+            {loadingModal && <div className={`fixed inset-0 z-40 h-[100vh] w-[100vw] ${topLevelBackdropClasses}`} />}
+
            {/* Sidebar */}
            <div className="sticky top-[-42px] z-20 min-w-[260px] grow-0 md:top-[-52px] tablet:fixed tablet:top-[8vmin] tablet:basis-[260px]">
                <div className='-mx-6 h-[84px] bg-white px-6 tablet:m-0 tablet:bg-transparent tablet:p-0'>
--- a/apps/admin-x-settings/src/admin-x-ds/global/modal/Modal.tsx
+++ b/apps/admin-x-settings/src/admin-x-ds/global/modal/Modal.tsx
@ -41,6 +41,8 @@ export interface ModalProps {
    formSheet?: boolean;
 }

+export const topLevelBackdropClasses = 'bg-[rgba(98,109,121,0.2)] backdrop-blur-[3px]';
+
 const Modal: React.FC<ModalProps> = ({
    size = 'md',
    testId,
@ -257,7 +259,7 @@ const Modal: React.FC<ModalProps> = ({
        <div className={backdropClasses} id='modal-backdrop' onClick={handleBackdropClick}>
            <div className={clsx(
                'pointer-events-none fixed inset-0 z-0',
-                (backDrop && !formSheet) && 'bg-[rgba(98,109,121,0.2)] backdrop-blur-[3px]',
+                (backDrop && !formSheet) && topLevelBackdropClasses,
                formSheet && 'bg-[rgba(98,109,121,0.08)]'
            )}></div>
            <section className={modalClasses} data-testid={testId} style={modalStyles}>
--- a/apps/admin-x-settings/src/admin-x-ds/settings/SettingNavItem.tsx
+++ b/apps/admin-x-settings/src/admin-x-ds/settings/SettingNavItem.tsx
@ -1,6 +1,6 @@
 import React from 'react';
 import clsx from 'clsx';
-import {useScrollSectionContext} from '../../hooks/useScrollSection';
+import {useScrollSectionContext, useScrollSectionNav} from '../../hooks/useScrollSection';
 import {useSearch} from '../../components/providers/ServiceProvider';

 interface Props {
@ -16,6 +16,7 @@ const SettingNavItem: React.FC<Props> = ({
    keywords,
    onClick = () => {}
 }) => {
+    const {ref, props} = useScrollSectionNav(navid);
    const {currentSection} = useScrollSectionContext();
    const {checkVisible} = useSearch();

@ -26,7 +27,7 @@ const SettingNavItem: React.FC<Props> = ({
    );

    return (
-        <li><button className={classNames} name={navid} type='button' onClick={onClick}>{title}</button></li>
+        <li ref={ref} {...props}><button className={classNames} name={navid} type='button' onClick={onClick}>{title}</button></li>
    );
 };

--- a/apps/admin-x-settings/src/components/Sidebar.tsx
+++ b/apps/admin-x-settings/src/components/Sidebar.tsx
@ -43,7 +43,7 @@ const Sidebar: React.FC = () => {
                <Icon className='absolute top-2 md:top-6 tablet:top-10' colorClass='text-grey-500' name='magnifying-glass' size='sm' />
                <TextField autoComplete="off" className='border-b border-grey-500 bg-transparent px-3 py-1.5 pl-[24px] text-sm dark:text-white' placeholder="Search" title="Search" value={filter} hideTitle unstyled onChange={updateSearch} />
            </div>
-            <div className="no-scrollbar hidden pt-10 tablet:!visible tablet:!block tablet:h-[calc(100vh-5vmin-84px-64px)] tablet:w-[240px] tablet:overflow-y-auto">
+            <div className="no-scrollbar hidden pt-10 tablet:!visible tablet:!block tablet:h-[calc(100vh-5vmin-84px-64px)] tablet:w-[240px] tablet:overflow-y-auto" id='admin-x-settings-sidebar'>
                <SettingNavSection keywords={Object.values(generalSearchKeywords).flat()} title="General">
                    <SettingNavItem keywords={generalSearchKeywords.titleAndDescription} navid='title-and-description' title="Title and description" onClick={handleSectionClick} />
                    <SettingNavItem keywords={generalSearchKeywords.timeZone} navid='timezone' title="Timezone" onClick={handleSectionClick} />
--- a/apps/admin-x-settings/src/components/providers/RoutingProvider.tsx
+++ b/apps/admin-x-settings/src/components/providers/RoutingProvider.tsx
@ -18,11 +18,13 @@ export type InternalLink = {
 export type RoutingContextData = {
    route: string;
    updateRoute: (to: string | InternalLink | ExternalLink) => void;
+    loadingModal: boolean;
 };

 export const RouteContext = createContext<RoutingContextData>({
    route: '',
-    updateRoute: () => {}
+    updateRoute: () => {},
+    loadingModal: false
 });

 export type RoutingModalProps = {
@ -108,13 +110,16 @@ const handleNavigation = () => {
    if (pathName) {
        const [path, modal] = Object.entries(modalPaths).find(([modalPath]) => matchRoute(pathName, modalPath)) || [];

-        if (path && modal) {
-            modal().then(({default: component}) => NiceModal.show(component, {params: matchRoute(pathName, path)}));
-        }
-
-        return pathName;
+        return {
+            pathName,
+            modal: (path && modal) ?
+                modal().then(({default: component}) => {
+                    NiceModal.show(component, {params: matchRoute(pathName, path)});
+                }) :
+                undefined
+        };
    }
-    return '';
+    return {pathName: ''};
 };

 const matchRoute = (pathname: string, routeDefinition: string) => {
@ -133,7 +138,8 @@ type RouteProviderProps = {
 };

 const RoutingProvider: React.FC<RouteProviderProps> = ({externalNavigate, children}) => {
-    const [route, setRoute] = useState<string>('');
+    const [route, setRoute] = useState<string | undefined>(undefined);
+    const [loadingModal, setLoadingModal] = useState(false);

    useEffect(() => {
        // Preload all the modals after initial render to avoid a delay when opening them
@ -161,12 +167,16 @@ const RoutingProvider: React.FC<RouteProviderProps> = ({externalNavigate, childr

    useEffect(() => {
        const handleHashChange = () => {
-            const matchedRoute = handleNavigation();
-            setRoute(matchedRoute);
+            const {pathName, modal} = handleNavigation();
+            setRoute(pathName);
+
+            if (modal) {
+                setLoadingModal(true);
+                modal.then(() => setLoadingModal(false));
+            }
        };

-        const matchedRoute = handleNavigation();
-        setRoute(matchedRoute);
+        handleHashChange();

        window.addEventListener('hashchange', handleHashChange);

@ -175,11 +185,16 @@ const RoutingProvider: React.FC<RouteProviderProps> = ({externalNavigate, childr
        };
    }, []); // eslint-disable-line react-hooks/exhaustive-deps

+    if (route === undefined) {
+        return null;
+    }
+
    return (
        <RouteContext.Provider
            value={{
                route,
-                updateRoute
+                updateRoute,
+                loadingModal
            }}
        >
            <ScrollSectionProvider navigatedSection={route.split('/')[0]}>
--- a/apps/admin-x-settings/src/components/settings/email/newsletters/NewsletterDetailModal.tsx
+++ b/apps/admin-x-settings/src/components/settings/email/newsletters/NewsletterDetailModal.tsx
@ -321,6 +321,7 @@ const Sidebar: React.FC<{
                        config={config}
                        hint='Any extra information or legal text'
                        nodes='MINIMAL_NODES'
+                        placeholder=' '
                        title='Email footer'
                        value={newsletter.footer_content || ''}
                        onChange={html => updateNewsletter({footer_content: html})}
--- a/apps/admin-x-settings/src/components/settings/membership/Tiers.tsx
+++ b/apps/admin-x-settings/src/components/settings/membership/Tiers.tsx
@ -35,14 +35,7 @@ const Tiers: React.FC<{ keywords: string[] }> = ({keywords}) => {
    };

    const sortTiers = (t: Tier[]) => {
-        t.sort((a, b) => {
-            if ((a.monthly_price as number) < (b.monthly_price as number)) {
-                return -1;
-            } else {
-                return 1;
-            }
-        });
-        return t;
+        return [...t].sort((a, b) => (a.monthly_price ?? 0) - (b.monthly_price ?? 0));
    };

    const tabs = [
--- a/apps/admin-x-settings/src/components/settings/membership/tiers/TierDetailModal.tsx
+++ b/apps/admin-x-settings/src/components/settings/membership/tiers/TierDetailModal.tsx
@ -244,6 +244,11 @@ const TierDetailModalContent: React.FC<{tier?: Tier}> = ({tier}) => {
                            value={benefits.newItem}
                            hideTitle
                            onChange={e => benefits.setNewItem(e.target.value)}
+                            onKeyDown={(e) => {
+                                if (e.key === 'Enter') {
+                                    benefits.addItem();
+                                }
+                            }}
                        />
                        <Button
                            className='absolute right-0 top-1'
--- a/apps/admin-x-settings/src/components/settings/site/ThemeModal.tsx
+++ b/apps/admin-x-settings/src/components/settings/site/ThemeModal.tsx
@ -14,8 +14,8 @@ import ThemeInstalledModal from './theme/ThemeInstalledModal';
 import ThemePreview from './theme/ThemePreview';
 import useRouting from '../../../hooks/useRouting';
 import {HostLimitError, useLimiter} from '../../../hooks/useLimiter';
+import {InstalledTheme, Theme, useBrowseThemes, useInstallTheme, useUploadTheme} from '../../../api/themes';
 import {OfficialTheme} from '../../providers/ServiceProvider';
-import {Theme, useBrowseThemes, useInstallTheme, useUploadTheme} from '../../../api/themes';

 interface ThemeToolbarProps {
    selectedTheme: OfficialTheme|null;
@ -235,49 +235,60 @@ const ChangeThemeModal = NiceModal.create(() => {
        return;
    }

-    let installedTheme;
+    let installedTheme: Theme|InstalledTheme|undefined;
    let onInstall;
    if (selectedTheme) {
        installedTheme = themes.find(theme => theme.name.toLowerCase() === selectedTheme!.name.toLowerCase());
        onInstall = async () => {
-            setInstalling(true);
-            const data = await installTheme(selectedTheme.ref);
-            setInstalling(false);
-
-            const newlyInstalledTheme = data.themes[0];
-
            let title = 'Success';
-            let prompt = <>
-                <strong>{newlyInstalledTheme.name}</strong> has been successfully installed.
-            </>;
+            let prompt = <></>;

-            if (!newlyInstalledTheme.active) {
+            // default theme can't be installed, only activated
+            if (selectedTheme.ref === 'default') {
+                title = 'Activate theme';
+                prompt = <>By clicking below, <strong>{selectedTheme.name}</strong> will automatically be activated as the theme for your site.</>;
+            } else {
+                setInstalling(true);
+                const data = await installTheme(selectedTheme.ref);
+                setInstalling(false);
+
+                const newlyInstalledTheme = data.themes[0];
+
+                title = 'Success';
                prompt = <>
-                    {prompt}{' '}
-                    Do you want to activate it now?
-                </>;
-            }
-
-            if (newlyInstalledTheme.errors?.length || newlyInstalledTheme.warnings?.length) {
-                const hasErrors = newlyInstalledTheme.errors?.length;
-
-                title = `Installed with ${hasErrors ? 'errors' : 'warnings'}`;
-                prompt = <>
-                    The theme <strong>&quot;{newlyInstalledTheme.name}&quot;</strong> was installed successfully but we detected some {hasErrors ? 'errors' : 'warnings'}.
+                    <strong>{newlyInstalledTheme.name}</strong> has been successfully installed.
                </>;

                if (!newlyInstalledTheme.active) {
                    prompt = <>
-                        {prompt}
-                        You are still able to activate and use the theme but it is recommended to contact the theme developer fix these {hasErrors ? 'errors' : 'warnings'} before you do so.
+                        {prompt}{' '}
+        Do you want to activate it now?
                    </>;
                }
+
+                if (newlyInstalledTheme.errors?.length || newlyInstalledTheme.warnings?.length) {
+                    const hasErrors = newlyInstalledTheme.errors?.length;
+
+                    title = `Installed with ${hasErrors ? 'errors' : 'warnings'}`;
+                    prompt = <>
+        The theme <strong>&quot;{newlyInstalledTheme.name}&quot;</strong> was installed successfully but we detected some {hasErrors ? 'errors' : 'warnings'}.
+                    </>;
+
+                    if (!newlyInstalledTheme.active) {
+                        prompt = <>
+                            {prompt}
+            You are still able to activate and use the theme but it is recommended to contact the theme developer fix these {hasErrors ? 'errors' : 'warnings'} before you do so.
+                        </>;
+                    }
+                }
+
+                installedTheme = newlyInstalledTheme;
            }

            NiceModal.show(ThemeInstalledModal, {
                title,
                prompt,
-                installedTheme: newlyInstalledTheme,
+                installedTheme: installedTheme!,
                onActivate: () => {
                    updateRoute('design/edit');
                    modal.remove();
@ -304,7 +315,6 @@ const ChangeThemeModal = NiceModal.create(() => {
                <div className='grow'>
                    {selectedTheme &&
                        <ThemePreview
-                            installButtonLabel={installedTheme ? `Update ${selectedTheme?.name}` : `Install ${selectedTheme?.name}`}
                            installedTheme={installedTheme}
                            isInstalling={isInstalling}
                            selectedTheme={selectedTheme}
--- a/apps/admin-x-settings/src/components/settings/site/theme/ThemePreview.tsx
+++ b/apps/admin-x-settings/src/components/settings/site/theme/ThemePreview.tsx
@ -14,7 +14,6 @@ const ThemePreview: React.FC<{
    selectedTheme?: OfficialTheme;
    isInstalling?: boolean;
    installedTheme?: Theme;
-    installButtonLabel?: string;
    onBack: () => void;
    onClose: () => void;
    onInstall?: () => void | Promise<void>;
@ -22,7 +21,6 @@ const ThemePreview: React.FC<{
    selectedTheme,
    isInstalling,
    installedTheme,
-    installButtonLabel,
    onBack,
    onClose,
    onInstall
@ -33,8 +31,18 @@ const ThemePreview: React.FC<{
        return null;
    }

+    let installButtonLabel = `Install ${selectedTheme.name}`;
+
+    if (isInstalling) {
+        installButtonLabel = 'Installing...';
+    } else if (selectedTheme.ref === 'default') {
+        installButtonLabel = `Activate ${selectedTheme.name}`;
+    } else if (installedTheme) {
+        installButtonLabel = `Update ${selectedTheme.name}`;
+    }
+
    const handleInstall = () => {
-        if (installedTheme) {
+        if (installedTheme && selectedTheme.ref !== 'default') {
            NiceModal.show(ConfirmationModal, {
                title: 'Overwrite theme',
                prompt: (
--- a/apps/admin-x-settings/src/hooks/useScrollSection.tsx
+++ b/apps/admin-x-settings/src/hooks/useScrollSection.tsx
@ -2,11 +2,13 @@ import {ReactNode, createContext, useCallback, useContext, useEffect, useMemo, u

 interface ScrollSectionContextData {
    updateSection: (id: string, element: HTMLDivElement) => void;
+    updateNav: (id: string, element: HTMLLIElement) => void;
    currentSection: string | null;
 }

 const ScrollSectionContext = createContext<ScrollSectionContextData>({
    updateSection: () => {},
+    updateNav: () => {},
    currentSection: null
 });

@ -14,16 +16,53 @@ export const useScrollSectionContext = () => useContext(ScrollSectionContext);

 const scrollMargin = 193;

-const scrollToSection = (element: HTMLDivElement) => {
+const scrollToSection = (element: HTMLDivElement, doneInitialScroll: boolean) => {
    const root = document.getElementById('admin-x-root')!;
    const top = element.getBoundingClientRect().top + root.scrollTop;

    root.scrollTo({
-        behavior: 'smooth',
+        behavior: doneInitialScroll ? 'smooth' : 'instant',
        top: top - scrollMargin
    });
 };

+const scrollSidebarNav = (navElement: HTMLLIElement, doneInitialScroll: boolean) => {
+    const sidebar = document.getElementById('admin-x-settings-sidebar')!;
+
+    const bounds = navElement.getBoundingClientRect();
+
+    const parentBounds = sidebar.getBoundingClientRect();
+    const offsetTop = parentBounds.top + 40;
+
+    if (bounds.top >= offsetTop && bounds.left >= parentBounds.left && bounds.right <= parentBounds.right && bounds.bottom <= parentBounds.bottom) {
+        return;
+    }
+
+    if (!['auto', 'scroll'].includes(getComputedStyle(sidebar).overflowY)) {
+        return;
+    }
+
+    const behavior = doneInitialScroll ? 'smooth' : 'instant';
+
+    // If this is the first nav item, scroll to top
+    if (sidebar.querySelector('[data-setting-nav-item]') === navElement) {
+        sidebar.scrollTo({
+            top: 0,
+            behavior
+        });
+    } else if (bounds.top < offsetTop) {
+        sidebar.scrollTo({
+            top: sidebar.scrollTop + bounds.top - offsetTop,
+            behavior
+        });
+    } else {
+        sidebar.scrollTo({
+            top: sidebar.scrollTop + bounds.top - parentBounds.top - parentBounds.height + bounds.height + 4,
+            behavior
+        });
+    }
+};
+
 export const ScrollSectionProvider: React.FC<{
    navigatedSection: string;
    children: ReactNode;
@ -31,7 +70,11 @@ export const ScrollSectionProvider: React.FC<{
    const sectionElements = useRef<Record<string, HTMLDivElement>>({});
    const [intersectingSections, setIntersectingSections] = useState<string[]>([]);
    const [lastIntersectedSection, setLastIntersectedSection] = useState<string | null>(null);
+
    const [doneInitialScroll, setDoneInitialScroll] = useState(false);
+    const [, setDoneSidebarScroll] = useState(false);
+
+    const navElements = useRef<Record<string, HTMLLIElement>>({});

    const intersectionObserver = useMemo(() => new IntersectionObserver((entries) => {
        const entriesWithId = entries.map(({isIntersecting, target}) => ({
@ -63,7 +106,7 @@ export const ScrollSectionProvider: React.FC<{
            return newSections;
        });
    }, {
-        rootMargin: `-${scrollMargin - 10}px 0px -40% 0px`
+        rootMargin: `-${scrollMargin - 50}px 0px -40% 0px`
    }), []);

    const updateSection = useCallback((id: string, element: HTMLDivElement) => {
@ -79,13 +122,15 @@ export const ScrollSectionProvider: React.FC<{
        intersectionObserver.observe(element);

        if (!doneInitialScroll && id === navigatedSection) {
-            scrollToSection(element);
-
-            // element.scrollIntoView({behavior: 'smooth'});
+            scrollToSection(element, false);
            setDoneInitialScroll(true);
        }
    }, [intersectionObserver, navigatedSection, doneInitialScroll]);

+    const updateNav = useCallback((id: string, element: HTMLLIElement) => {
+        navElements.current[id] = element;
+    }, []);
+
    const currentSection = useMemo(() => {
        if (navigatedSection && intersectingSections.includes(navigatedSection)) {
            return navigatedSection;
@ -100,14 +145,26 @@ export const ScrollSectionProvider: React.FC<{

    useEffect(() => {
        if (navigatedSection && sectionElements.current[navigatedSection]) {
-            scrollToSection(sectionElements.current[navigatedSection]);
-            setDoneInitialScroll(true);
+            setDoneInitialScroll((done) => {
+                scrollToSection(sectionElements.current[navigatedSection], done);
+                return true;
+            });
        }
    }, [navigatedSection]);

+    useEffect(() => {
+        if (currentSection && navElements.current[currentSection]) {
+            setDoneSidebarScroll((done) => {
+                scrollSidebarNav(navElements.current[currentSection], done);
+                return true;
+            });
+        }
+    }, [currentSection]);
+
    return (
        <ScrollSectionContext.Provider value={{
            updateSection,
+            updateNav,
            currentSection
        }}>
            {children}
@ -129,3 +186,19 @@ export const useScrollSection = (id?: string) => {
        ref
    };
 };
+
+export const useScrollSectionNav = (id?: string) => {
+    const {updateNav} = useScrollSectionContext();
+    const ref = useRef<HTMLLIElement>(null);
+
+    useEffect(() => {
+        if (id && ref.current) {
+            updateNav(id, ref.current);
+        }
+    }, [id, updateNav]);
+
+    return {
+        ref,
+        props: {'data-setting-nav-item': true}
+    };
+};
--- a/apps/admin-x-settings/test/e2e/site/theme.test.ts
+++ b/apps/admin-x-settings/test/e2e/site/theme.test.ts
@ -40,7 +40,7 @@ test.describe('Theme settings', async () => {

        await modal.getByRole('button', {name: /Casper/}).click();

-        await expect(modal.getByRole('button', {name: 'Update Casper'})).toBeVisible();
+        await expect(modal.getByRole('button', {name: 'Activate Casper'})).toBeVisible();

        await expect(page.locator('iframe[title="Theme preview"]')).toHaveAttribute('src', 'https://demo.ghost.io/');