Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Utility hooks additions #867

Merged
merged 8 commits into from
Feb 10, 2025
3 changes: 2 additions & 1 deletion docs/app/docs/layout.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,8 @@ const Layout = ({ children }: any) => {
<Navigation />
</div>
<div className='lg:px-4 text-gray-1000 flex-1 flex flex-col gap-4 overflow-y-scroll pt-2' id="docs-content">

<div className='mt-2 max-w-[1440px] mx-auto' >
<Callout color="green">
<div className='flex-none'>
<InfoIcon />
Expand All @@ -23,7 +25,6 @@ const Layout = ({ children }: any) => {
</div>
</div>
</Callout>
<div className='mt-2' >
{children}
</div>
</div>
Expand Down
6 changes: 3 additions & 3 deletions docs/app/testing/full_page_scroll/page.tsx
Original file line number Diff line number Diff line change
@@ -1,11 +1,11 @@
import FullHeightScroll from "@/components/layout/ScrollContainers/FullHeightScroll";

const FullPageScroll = () => {
return <FullHeightScroll scrollable={true}>
<div className="w-full h-full bg-gray-200">
return <FullHeightScroll scrollable={true} className="bg-gray-200 p-10 pb-20">
<div>
{
Array.from({ length: 100 }).map((_, index) => (
<div key={index} className="w-full h-10 bg-gray-200">
<div key={index} className="text-gray-1000">
{index}
</div>
))
Expand Down
2 changes: 1 addition & 1 deletion docs/components/layout/Documentation/Documentation.js
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@ const Documentation = ({ title = '', description = '', currentPage = undefined,
const DocsTable = ({ children, columns = [], data = [] }) => {
return <div className='mb-20 max-w-screen-md'>
<BookMarkLink id="api-documentation"> <Heading as="h6" className="mb-4">API Documentation</Heading> </BookMarkLink>
<Table columns={columns} data={data} >
<Table columns={columns} data={data} color="gray" >
{children}
</Table>
</div>;
Expand Down
10 changes: 7 additions & 3 deletions docs/components/layout/ScrollContainers/FullHeightScroll.tsx
Original file line number Diff line number Diff line change
@@ -1,7 +1,11 @@
const FullHeightScroll = ({ children, scrollable = true }) => {
const FullHeightScroll = ({ children, scrollable = true, className='', ...props }) => {
// An important layout component that allows for full height scrolling
return <div className={`flex flex-1 ${scrollable?'overflow-y-auto overflow-x-hidden':''}`}>
{children}
return <div className={` ${className} ${scrollable?' overflow-y-auto overflow-x-hidden':''}`} {...props} style={{

}}>
<div className="max-w-[1440px] mx-auto">
{children}
</div>
</div>
}

Expand Down
59 changes: 59 additions & 0 deletions src/core/hooks/composeEventHandlers/composeEventHandlers.test.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,59 @@
import React from 'react';
import { render, screen, fireEvent } from '@testing-library/react';
import composeEventHandlers from './index';

const OnlyOriginalHandlerWithPreventDefault = ({
checkForDefaultPrevented = true
}: {
checkForDefaultPrevented?: boolean;
}) => {
const originalClickHandler = (event: React.MouseEvent<HTMLButtonElement>) => {
event.preventDefault();
// we prevent default, so we should not see our handler
console.log('RETURNING_ORIGINAL_HANDLER');
};
const ourClickHandler = () => {
// This won't be triggered because we prevent default in the original handler
console.log('RETURNING_OUR_HANDLER');

console.log('RETURN_OUR_HANDLER_WITH_CHECK_FOR_DEFAULT_PREVENTED');
};
const composedHandleClick = composeEventHandlers(
originalClickHandler,
ourClickHandler,
{ checkForDefaultPrevented }
);

return <button onClick={composedHandleClick}>Click Me</button>;
};

describe('composeEventHandlers', () => {
let consoleLogSpy: jest.SpyInstance;

beforeEach(() => {
consoleLogSpy = jest.spyOn(console, 'log').mockImplementation(() => {});
});

afterEach(() => {
consoleLogSpy.mockRestore();
});

test('should compose event handlers', () => {
render(<OnlyOriginalHandlerWithPreventDefault />);
const button = screen.getByText('Click Me');
fireEvent.click(button);
expect(consoleLogSpy).toHaveBeenCalledWith('RETURNING_ORIGINAL_HANDLER');
expect(consoleLogSpy).not.toHaveBeenCalledWith('RETURNING_OUR_HANDLER');
});

test('should compose event handlers with checkForDefaultPrevented false', () => {
// if checkForDefaultPrevented is false, we should see our handler
render(<OnlyOriginalHandlerWithPreventDefault checkForDefaultPrevented={false} />);
const button = screen.getByText('Click Me');
fireEvent.click(button);
expect(consoleLogSpy).toHaveBeenCalledWith('RETURNING_ORIGINAL_HANDLER');

// even if the event is prevented, we should see our handler - the function completely ignores the event prevent default
expect(consoleLogSpy).toHaveBeenCalledWith('RETURN_OUR_HANDLER_WITH_CHECK_FOR_DEFAULT_PREVENTED');
});
});
24 changes: 24 additions & 0 deletions src/core/hooks/composeEventHandlers/index.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
type EventHandler<E = React.SyntheticEvent> = (event: E) => void;

// From Radix UI - packages/core/primitive/src/primitive.tsx

const composeEventHandlers = <E extends React.SyntheticEvent>(
originalEventHandler?: EventHandler<E>,
ourEventHandler?: EventHandler<E>,
{ checkForDefaultPrevented = true } = {}
) => {
// Returns a function that handles the event
return function handleEvent(event: E) {
// If the original event handler is defined, call it
if (typeof originalEventHandler === 'function') {
originalEventHandler(event);
}

// If the default prevented flag is false or the event is not prevented, call our event handler
if (checkForDefaultPrevented === false || !event.defaultPrevented) {
return ourEventHandler?.(event);
}
};
};

export default composeEventHandlers;
14 changes: 14 additions & 0 deletions src/core/hooks/useLayoutEffect/index.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
// A simple wrapper for useLayoutEffect that does not throw an error on server components

import { useLayoutEffect as ReactUseLayoutEffect } from 'react';

// When we use useLayoutEffect in a server component, it will throw an error.
// One of the hooks that do not work on server components
// wrapping it this way and using it will make sure errors are not thrown and fail silently
// If it's server, we just return a noop function (no operation)

// Radix UI does it this way - https://github.com/radix-ui/primitives/blob/main/packages/react/use-layout-effect/src/use-layout-effect.tsx
//
const useLayoutEffect = globalThis.document ? ReactUseLayoutEffect : () => {};

export default useLayoutEffect;
37 changes: 37 additions & 0 deletions src/core/hooks/useLayoutEffect/useLayoutEffect.test.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
import React from 'react';
import useLayoutEffect from './index';
import { render } from '@testing-library/react';

const TestComponent = () => {
// invoke useLayoutEffect and check if the component still mounts as expected
useLayoutEffect(() => {
// mounts
}, []);
return <div>Hello</div>;
};

describe('useLayoutEffect', () => {
it('should mount without errors in SSR environment', () => {
render(<TestComponent />);
});

it('should execute effect in browser environment', () => {
const mockFn = jest.fn();
const TestComponent = () => {
useLayoutEffect(() => {
mockFn();
}, []);
return <div>Hello</div>;
};

// Mock document existence
const originalDocument = global.document;
global.document = {} as typeof document;

render(<TestComponent />);
expect(mockFn).toHaveBeenCalledTimes(1);

// Cleanup
global.document = originalDocument;
});
Comment on lines +18 to +36
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Improve browser environment test robustness.

The browser environment test could be more robust with these improvements:

  1. Use proper test lifecycle hooks for setup/cleanup
  2. Use a more realistic document mock
  3. Add error handling for cleanup
+    let originalDocument: typeof document;
+
+    beforeEach(() => {
+        originalDocument = global.document;
+        // Mock a more realistic document object
+        global.document = {
+            createElement: jest.fn(),
+            body: {},
+        } as unknown as typeof document;
+    });
+
+    afterEach(() => {
+        try {
+            global.document = originalDocument;
+        } catch (error) {
+            console.error('Failed to restore document:', error);
+            throw error;
+        }
+    });
+
     it('should execute effect in browser environment', () => {
         const mockFn = jest.fn();
         const TestComponent = () => {
             useLayoutEffect(() => {
                 mockFn();
             }, []);
             return <div>Hello</div>;
         };
 
-        // Mock document existence
-        const originalDocument = global.document;
-        global.document = {} as typeof document;
-
         render(<TestComponent />);
         expect(mockFn).toHaveBeenCalledTimes(1);
-
-        // Cleanup
-        global.document = originalDocument;
     });
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
it('should execute effect in browser environment', () => {
const mockFn = jest.fn();
const TestComponent = () => {
useLayoutEffect(() => {
mockFn();
}, []);
return <div>Hello</div>;
};
// Mock document existence
const originalDocument = global.document;
global.document = {} as typeof document;
render(<TestComponent />);
expect(mockFn).toHaveBeenCalledTimes(1);
// Cleanup
global.document = originalDocument;
});
let originalDocument: typeof document;
beforeEach(() => {
originalDocument = global.document;
// Mock a more realistic document object
global.document = {
createElement: jest.fn(),
body: {},
} as unknown as typeof document;
});
afterEach(() => {
try {
global.document = originalDocument;
} catch (error) {
console.error('Failed to restore document:', error);
throw error;
}
});
it('should execute effect in browser environment', () => {
const mockFn = jest.fn();
const TestComponent = () => {
useLayoutEffect(() => {
mockFn();
}, []);
return <div>Hello</div>;
};
render(<TestComponent />);
expect(mockFn).toHaveBeenCalledTimes(1);
});

});
15 changes: 8 additions & 7 deletions src/core/primitives/Toggle/index.tsx
Original file line number Diff line number Diff line change
@@ -1,12 +1,15 @@
import React, { useState } from 'react';

import Primitive from '~/core/primitives/Primitive';
import composeEventHandlers from '~/core/hooks/composeEventHandlers';

export interface TogglePrimitiveProps {
defaultPressed?: boolean;
pressed?: boolean;
children?: React.ReactNode;
className?: string;
onClick?: (event: React.MouseEvent<HTMLButtonElement>) => void;
onKeyDown?: (event: React.KeyboardEvent<HTMLButtonElement>) => void;
label?: string;
disabled?: boolean;
onPressedChange?: (isPressed: boolean) => void;
Expand All @@ -28,7 +31,7 @@ const TogglePrimitive = ({
const isControlled = controlledPressed !== undefined;
const isPressed = isControlled ? controlledPressed : uncontrolledPressed;

const handlePressed = () => {
const handleTogglePressed = () => {
if (disabled) {
return;
}
Expand All @@ -40,12 +43,10 @@ const TogglePrimitive = ({
onPressedChange(updatedPressed);
};

const handleKeyDown = (event: React.KeyboardEvent) => {
// TODO: Should these be handled by the browser?
// Or should we add these functionalities inside the ButtonPrimitive?
const handleKeyDown = (event: any) => {
if (event.key === ' ' || event.key === 'Enter') {
event.preventDefault();
handlePressed();
handleTogglePressed();
}
};

Expand All @@ -54,8 +55,8 @@ const TogglePrimitive = ({
ariaAttributes['aria-disabled'] = disabled ? 'true' : 'false';

return <Primitive.button
onClick={handlePressed}
onKeyDown={handleKeyDown}
onClick={composeEventHandlers(props.onClick, handleTogglePressed)}
onKeyDown={composeEventHandlers(props.onKeyDown, handleKeyDown)}
data-state={isPressed ? 'on' : 'off'}
disabled={disabled}
{...ariaAttributes}
Expand Down
25 changes: 14 additions & 11 deletions styles/themes/components/table.scss
Original file line number Diff line number Diff line change
@@ -1,7 +1,5 @@
.rad-ui-table-wrapper{

border: 1px solid var(--rad-ui-color-accent-400);
box-shadow: 0 2px 20px 20px var(--rad-ui-color-accent-50);
border: 1px solid var(--rad-ui-color-accent-500);
border-radius: 12px;
overflow: hidden;

Expand All @@ -15,31 +13,36 @@
color: var(--rad-ui-color-gray-1000);

.header{
padding: 0.75rem 1rem;
padding: 12px;
vertical-align: inherit;
font-weight: 700;
font-size:14px;
border-bottom: 1px solid var(--rad-ui-color-accent-500);

.cell-header{
padding: 0.75rem 1rem;
padding: 12px;
vertical-align: inherit;
font-weight: 700;
background-color: var(--rad-ui-color-accent-100);
font-weight: 500;
background-color: var(--rad-ui-color-accent-200);
}
}

.row{
vertical-align: inherit;
border-bottom: 1px solid var(--rad-ui-color-accent-400);

border-bottom: 1px solid var(--rad-ui-color-accent-300);
background-color: var(--rad-ui-color-accent-50);

&:last-child{
border-bottom: none;
}
}

.cell{
padding: 0.75rem 1rem;
vertical-align: inherit;
border-bottom: 1px solid var(--rad-ui-color-accent-400);
color: var(--rad-ui-color-gray-950);
color: var(--rad-ui-color-gray-1000);
font-weight: 300;
font-size: 14px;
}
}

Expand Down
Loading