-
-
Notifications
You must be signed in to change notification settings - Fork 32
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
Feature toggle primitive #553
Changes from all commits
a94b1e6
67bf31f
7bac0c5
6722af2
6830fd8
0dc30de
3169d8b
27b6677
c642bd0
4ce321c
90852e5
b4786f9
fe3b0c9
50f93ed
3eeb310
b887d51
2d4687f
58f70f9
57fcc01
683ed66
b7f6db4
9829aeb
bb80175
1c3cfdc
2212cc8
3f49aaf
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,8 @@ | ||
import { createContext } from "react"; | ||
|
||
interface TogglePrimitiveContextType { | ||
isPressed: boolean | undefined; | ||
handlePressed: () => void; | ||
|
||
} | ||
export const TogglePrimitiveContext = createContext<TogglePrimitiveContextType>({} as TogglePrimitiveContextType) | ||
kotAPI marked this conversation as resolved.
Show resolved
Hide resolved
|
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
@@ -0,0 +1,24 @@ | ||||||||||||||||||||||||||||||||||||||||||||||||
import React, { useState } from "react"; | ||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||
export interface TogglePrimitiveRootProps { | ||||||||||||||||||||||||||||||||||||||||||||||||
defaultPressed? : boolean | false; | ||||||||||||||||||||||||||||||||||||||||||||||||
pressed: boolean; | ||||||||||||||||||||||||||||||||||||||||||||||||
children?: React.ReactNode; | ||||||||||||||||||||||||||||||||||||||||||||||||
className?: string; | ||||||||||||||||||||||||||||||||||||||||||||||||
onChange : (isPressed:boolean) => void; | ||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||||||
Comment on lines
+3
to
+10
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fix prop types and add documentation The interface needs improvements for better type safety and documentation: +/**
+ * Props for the Toggle Primitive component that handles basic toggle functionality
+ */
export interface TogglePrimitiveRootProps {
- defaultPressed? : boolean | false;
+ /** Initial pressed state for uncontrolled usage */
+ defaultPressed?: boolean;
+ /** Current pressed state for controlled usage. Takes precedence over defaultPressed */
pressed: boolean;
+ /** Content to be rendered within the toggle */
children?: React.ReactNode;
+ /** Additional CSS classes */
className?: string;
+ /** Callback fired when toggle state changes */
onChange : (isPressed:boolean) => void;
} 📝 Committable suggestion
Suggested change
|
||||||||||||||||||||||||||||||||||||||||||||||||
const TogglePrimitiveRoot = ({children,className='',defaultPressed,pressed,onChange,...props}:TogglePrimitiveRootProps) => { | ||||||||||||||||||||||||||||||||||||||||||||||||
const [isPressed, setIsPressed] = useState(pressed || defaultPressed); | ||||||||||||||||||||||||||||||||||||||||||||||||
Comment on lines
+11
to
+12
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fix state management pattern The current state management mixes controlled and uncontrolled patterns incorrectly: -const TogglePrimitiveRoot = ({children,className='',defaultPressed,pressed,onChange,...props}:TogglePrimitiveRootProps) => {
- const [isPressed, setIsPressed] = useState(pressed || defaultPressed);
+const TogglePrimitiveRoot = ({
+ children,
+ className = '',
+ defaultPressed = false,
+ pressed,
+ onChange,
+ ...props
+}: TogglePrimitiveRootProps) => {
+ const [uncontrolledPressed, setUncontrolledPressed] = useState(defaultPressed);
+ const isPressed = pressed ?? uncontrolledPressed; 📝 Committable suggestion
Suggested change
|
||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||
const handlePressed = () => { | ||||||||||||||||||||||||||||||||||||||||||||||||
const updatedPressed = !isPressed; | ||||||||||||||||||||||||||||||||||||||||||||||||
setIsPressed(updatedPressed); | ||||||||||||||||||||||||||||||||||||||||||||||||
onChange(updatedPressed) | ||||||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||||||
Comment on lines
+14
to
+18
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Improve event handling The toggle needs proper event handling for both mouse and keyboard interactions: - const handlePressed = () => {
+ const handlePressed = (event: React.MouseEvent | React.KeyboardEvent) => {
+ if (
+ event.type === 'keydown' &&
+ (event as React.KeyboardEvent).key !== 'Enter' &&
+ (event as React.KeyboardEvent).key !== ' '
+ ) {
+ return;
+ }
+ event.preventDefault();
const updatedPressed = !isPressed;
- setIsPressed(updatedPressed);
+ if (pressed === undefined) {
+ setUncontrolledPressed(updatedPressed);
+ }
onChange(updatedPressed);
} 📝 Committable suggestion
Suggested change
|
||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||
return <span className={className} onClick={handlePressed} {...props}>{children}</span> | ||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||
}; | ||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||
export default TogglePrimitiveRoot; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,7 @@ | ||
import TogglePrimitiveRoot from "./fragments/TogglePrimitiveRoot" | ||
|
||
const TogglePrimitive = { | ||
Root: TogglePrimitiveRoot, | ||
} | ||
|
||
export default TogglePrimitive |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,19 @@ | ||
import React from "react"; | ||
import TogglePrimitive from "../index"; | ||
import SandboxEditor from "~/components/tools/SandboxEditor/SandboxEditor"; | ||
|
||
export default { | ||
title: 'Primitives/TogglePrimitive', | ||
component: TogglePrimitive, | ||
render: (args:any) => <SandboxEditor> | ||
|
||
<TogglePrimitive.Root {...args}> | ||
</TogglePrimitive.Root> | ||
</SandboxEditor> | ||
} | ||
|
||
export const All = { | ||
args: { | ||
className: '' | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Fix accessibility and simplify attribute handling
aria-pressed
attribute should reflect the actual state of the toggle, not just thepressed
prop.data-disabled
implementation can be simplified.📝 Committable suggestion