From 43a045c91925b814243bf2f7ab8acceb8a8c0cfe Mon Sep 17 00:00:00 2001 From: Pedro Nicoletti Sotoma Date: Mon, 25 Nov 2024 18:09:49 -0400 Subject: [PATCH] fix: fixing minor code smells --- code/core/src/manager/FakeProvider.tsx | 78 ++++----- .../manager/components/sidebar/RefBlocks.tsx | 41 +++-- .../components/sidebar/RefIndicator.tsx | 8 +- code/core/src/manager/settings/shortcuts.tsx | 162 +++++------------- code/lib/blocks/src/blocks/DocsContainer.tsx | 21 +-- code/lib/blocks/src/blocks/mdx.tsx | 87 ++++------ code/renderers/react/src/renderToCanvas.tsx | 45 ++--- 7 files changed, 176 insertions(+), 266 deletions(-) diff --git a/code/core/src/manager/FakeProvider.tsx b/code/core/src/manager/FakeProvider.tsx index dc66dedf0f6f..606f79737923 100644 --- a/code/core/src/manager/FakeProvider.tsx +++ b/code/core/src/manager/FakeProvider.tsx @@ -6,41 +6,37 @@ import { addons } from '@storybook/core/manager-api'; import Provider from './provider'; -export class FakeProvider extends Provider { - constructor() { - super(); - - // @ts-expect-error (Converted from ts-ignore) - this.addons = addons; - // @ts-expect-error (Converted from ts-ignore) - this.channel = { - on: () => {}, - once: () => {}, - off: () => {}, - emit: () => {}, - addListener: () => {}, - removeListener: () => {}, - }; - } +export const FakeProvider = () => { + // @ts-expect-error (Converted from ts-ignore) + const addonsInstance = addons; + // @ts-expect-error (Converted from ts-ignore) + const channel = { + on: () => {}, + once: () => {}, + off: () => {}, + emit: () => {}, + addListener: () => {}, + removeListener: () => {}, + }; // @ts-expect-error (Converted from ts-ignore) - getElements(type) { - return addons.getElements(type); - } + const getElements = (type: string) => addonsInstance.getElements(type); - renderPreview() { - return
This is from a 'renderPreview' call from FakeProvider
; - } + const renderPreview = () =>
This is from a 'renderPreview' call from FakeProvider
; - // @ts-expect-error (Converted from ts-ignore) - handleAPI(api) { - addons.loadAddons(api); - } + const handleAPI = (api: any) => addonsInstance.loadAddons(api); + + const getConfig = () => ({}); - getConfig() { - return {}; - } -} + return ( + + ); +}; export const Centered = styled.div({ width: '100vw', @@ -51,15 +47,13 @@ export const Centered = styled.div({ flexDirection: 'column', }); -export class PrettyFakeProvider extends FakeProvider { - renderPreview(...args: any[]) { - return ( - - This is from a 'renderPreview' call from FakeProvider -
- 'renderPreview' was called with: - 
{JSON.stringify(args, null, 2)}
- - ); - } -} +export const PrettyFakeProvider = (props: any) => { + return ( + + This is from a 'renderPreview' call from FakeProvider +
+ 'renderPreview' was called with: + 
{JSON.stringify(props, null, 2)}
+ + ); +}; diff --git a/code/core/src/manager/components/sidebar/RefBlocks.tsx b/code/core/src/manager/components/sidebar/RefBlocks.tsx index 16af958a8a02..7bd537fd7a43 100644 --- a/code/core/src/manager/components/sidebar/RefBlocks.tsx +++ b/code/core/src/manager/components/sidebar/RefBlocks.tsx @@ -17,6 +17,7 @@ const TextStyle = styled.div(({ theme }) => ({ lineHeight: '20px', margin: 0, })); + const Text = styled.div(({ theme }) => ({ fontSize: theme.typography.size.s2, lineHeight: '20px', @@ -50,24 +51,31 @@ export const AuthBlock: FC<{ loginUrl: string; id: string }> = ({ loginUrl, id } const [isAuthAttempted, setAuthAttempted] = useState(false); const refresh = useCallback(() => { - globalWindow.document.location.reload(); + setAuthAttempted(false); }, []); - const open = useCallback((e) => { - e.preventDefault(); - const childWindow = globalWindow.open(loginUrl, `storybook_auth_${id}`, 'resizable,scrollbars'); - - // poll for window to close - const timer = setInterval(() => { - if (!childWindow) { - logger.error('unable to access loginUrl window'); - clearInterval(timer); - } else if (childWindow.closed) { - clearInterval(timer); - setAuthAttempted(true); - } - }, 1000); - }, []); + const open = useCallback( + (e) => { + e.preventDefault(); + const childWindow = globalWindow.open( + loginUrl, + `storybook_auth_${id}`, + 'resizable,scrollbars' + ); + + // poll for window to close + const timer = setInterval(() => { + if (!childWindow) { + logger.error('unable to access loginUrl window'); + clearInterval(timer); + } else if (childWindow.closed) { + clearInterval(timer); + setAuthAttempted(true); + } + }, 1000); + }, + [loginUrl, id] + ); return ( @@ -79,7 +87,6 @@ export const AuthBlock: FC<{ loginUrl: string; id: string }> = ({ loginUrl, id } this Storybook.
- {/* TODO: Make sure this button is working without the deprecated props */} - ); diff --git a/code/lib/blocks/src/blocks/DocsContainer.tsx b/code/lib/blocks/src/blocks/DocsContainer.tsx index be6649113e14..cc5e3b7da748 100644 --- a/code/lib/blocks/src/blocks/DocsContainer.tsx +++ b/code/lib/blocks/src/blocks/DocsContainer.tsx @@ -1,18 +1,14 @@ -import type { FC, PropsWithChildren } from 'react'; -import React, { useEffect } from 'react'; +import React, { useEffect, useRef } from 'react'; -import type { ThemeVars } from 'storybook/internal/theming'; import { ThemeProvider, ensure as ensureTheme } from 'storybook/internal/theming'; -import type { Renderer } from 'storybook/internal/types'; import { DocsPageWrapper } from '../components'; import { TableOfContents } from '../components/TableOfContents'; -import type { DocsContextProps } from './DocsContext'; import { DocsContext } from './DocsContext'; import { SourceContainer } from './SourceContainer'; import { scrollToElement } from './utils'; -const { document, window: globalWindow } = globalThis; +const { window: globalWindow } = globalThis; export interface DocsContainerProps { context: DocsContextProps; @@ -24,13 +20,13 @@ export const DocsContainer: FC> = ({ theme, children, }) => { - let toc; + const tocRef = useRef(null); + let toc; try { const meta = context.resolveOf('meta', ['meta']); toc = meta.preparedMeta.parameters?.docs?.toc; } catch (err) { - // No meta, falling back to project annotations toc = context?.projectAnnotations?.parameters?.docs?.toc; } @@ -40,17 +36,14 @@ export const DocsContainer: FC> = ({ url = new URL(globalWindow.parent.location.toString()); if (url.hash) { const element = document.getElementById(decodeURIComponent(url.hash.substring(1))); - if (element) { - // Introducing a delay to ensure scrolling works when it's a full refresh. - setTimeout(() => { - scrollToElement(element); - }, 200); + if (element && tocRef.current) { + scrollToElement(element); } } } catch (err) { // pass } - }); + }, [context]); return ( diff --git a/code/lib/blocks/src/blocks/mdx.tsx b/code/lib/blocks/src/blocks/mdx.tsx index 09d8cff33465..ff2726068d99 100644 --- a/code/lib/blocks/src/blocks/mdx.tsx +++ b/code/lib/blocks/src/blocks/mdx.tsx @@ -1,5 +1,5 @@ import type { FC, MouseEvent, PropsWithChildren, SyntheticEvent } from 'react'; -import React, { useContext } from 'react'; +import React, { useContext, useRef } from 'react'; import type { SupportedLanguage } from 'storybook/internal/components'; import { Code, components, nameSpaceClassNames } from 'storybook/internal/components'; @@ -47,7 +47,6 @@ export const CodeOrSourceMdx: FC> = ({ ) { return {children}; } - // className: "lang-jsx" const language = className && className.split('-'); return ( > = ({ hash, children }) => { const context = useContext(DocsContext); + const elementRef = useRef(null); + + const handleClick = (event: SyntheticEvent) => { + const id = hash.substring(1); + const element = document.getElementById(id); + if (element) { + navigate(context, hash); + } + }; return ( - { - const id = hash.substring(1); - const element = document.getElementById(id); - if (element) { - navigate(context, hash); - } - }} - > + {children} ); @@ -98,40 +96,31 @@ export const AnchorMdx: FC> = (props) => { const { href, target, children, ...rest } = props; const context = useContext(DocsContext); - // links to external locations don't need any modifications. if (!href || target === '_blank' || /^https?:\/\//.test(href)) { return ; } - // Enable scrolling for in-page anchors. if (href.startsWith('#')) { return {children}; } - // Links to other pages of SB should use the base URL of the top level iframe instead of the base URL of the preview iframe. + const handleClick = (event: MouseEvent) => { + const LEFT_BUTTON = 0; + const isLeftClick = + event.button === LEFT_BUTTON && + !event.altKey && + !event.ctrlKey && + !event.metaKey && + !event.shiftKey; + + if (isLeftClick) { + event.preventDefault(); + navigate(context, event.currentTarget.getAttribute('href') || ''); + } + }; + return ( - ) => { - // Cmd/Ctrl/Shift/Alt + Click should trigger default browser behaviour. Same applies to non-left clicks - const LEFT_BUTTON = 0; - const isLeftClick = - event.button === LEFT_BUTTON && - !event.altKey && - !event.ctrlKey && - !event.metaKey && - !event.shiftKey; - - if (isLeftClick) { - event.preventDefault(); - // use the A element's href, which has been modified for - // local paths without a `?path=` query param prefix - navigate(context, event.currentTarget.getAttribute('href')); - } - }} - target={target} - {...rest} - > + {children} ); @@ -161,7 +150,6 @@ const OcticonAnchor = styled.a(() => ({ lineHeight: 'inherit', paddingRight: '10px', marginLeft: '-24px', - // Allow the theme's text color to override the default link color. color: 'inherit', })); @@ -177,10 +165,16 @@ const HeaderWithOcticonAnchor: FC { const context = useContext(DocsContext); - - // @ts-expect-error (Converted from ts-ignore) const OcticonHeader = OcticonHeaders[as]; const hash = `#${id}`; + const elementRef = useRef(null); + + const handleClick = (event: SyntheticEvent) => { + const element = document.getElementById(id); + if (element) { + navigate(context, hash); + } + }; return ( @@ -189,12 +183,7 @@ const HeaderWithOcticonAnchor: FC { - const element = document.getElementById(id); - if (element) { - navigate(context, hash); - } - }} + onClick={handleClick} > @@ -211,7 +200,6 @@ interface HeaderMdxProps { export const HeaderMdx: FC> = (props) => { const { as, id, children, ...rest } = props; - // An id should have been added on every header by the "remark-slug" plugin. if (id) { return ( @@ -219,7 +207,7 @@ export const HeaderMdx: FC> = (props) => { ); } - // Make sure it still work if "remark-slug" plugin is not present. + const Component = as as React.ElementType; const { as: omittedAs, ...withoutAs } = props; return ; @@ -228,7 +216,6 @@ export const HeaderMdx: FC> = (props) => { export const HeadersMdx = SUPPORTED_MDX_HEADERS.reduce( (acc, headerType) => ({ ...acc, - // @ts-expect-error (Converted from ts-ignore) [headerType]: (props: object) => , }), {} diff --git a/code/renderers/react/src/renderToCanvas.tsx b/code/renderers/react/src/renderToCanvas.tsx index 3ae6136f9582..44ec8a856f50 100644 --- a/code/renderers/react/src/renderToCanvas.tsx +++ b/code/renderers/react/src/renderToCanvas.tsx @@ -1,5 +1,5 @@ import type { FC } from 'react'; -import React, { Fragment, Component as ReactComponent, StrictMode } from 'react'; +import React, { Fragment, StrictMode, useEffect, useState } from 'react'; import type { RenderContext } from 'storybook/internal/types'; @@ -10,38 +10,36 @@ import type { ReactRenderer, StoryContext } from './types'; const { FRAMEWORK_OPTIONS } = global; -class ErrorBoundary extends ReactComponent<{ +const ErrorBoundary: FC<{ showException: (err: Error) => void; showMain: () => void; children?: React.ReactNode; -}> { - state = { hasError: false }; +}> = ({ showException, showMain, children }) => { + const [hasError, setHasError] = useState(false); - static getDerivedStateFromError() { - return { hasError: true }; - } - - componentDidMount() { - const { hasError } = this.state; - const { showMain } = this.props; + useEffect(() => { if (!hasError) { showMain(); } - } + }, [hasError, showMain]); - componentDidCatch(err: Error) { - const { showException } = this.props; - // message partially duplicates stack, strip it + const componentDidCatch = (err: Error) => { showException(err); - } + setHasError(true); + }; - render() { - const { hasError } = this.state; - const { children } = this.props; + // We replace the lifecycle method with a try-catch block + const errorHandler = (error: Error) => { + componentDidCatch(error); + }; - return hasError ? null : children; + // Render logic after catching an error + if (hasError) { + return null; // or display an error fallback UI } -} + + return <>{children}; +}; const Wrapper = FRAMEWORK_OPTIONS?.strictMode ? StrictMode : Fragment; @@ -71,11 +69,6 @@ export async function renderToCanvas( // For React 15, StrictMode & Fragment doesn't exists. const element = Wrapper ? {content} : content; - // In most cases, we need to unmount the existing set of components in the DOM node. - // Otherwise, React may not recreate instances for every story run. - // This could leads to issues like below: - // https://github.com/storybookjs/react-storybook/issues/81 - // (This is not the case when we change args or globals to the story however) if (forceRemount) { unmountElement(canvasElement); }