feat: add hex color code box #362
Conversation
|
@harini-1597 is attempting to deploy a commit to the Sanchit Bajaj's projects Team on Vercel. A member of the Team first needs to authorize it. |
WalkthroughThe changes to the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant ColorPicker
participant State
User->>+ColorPicker: Enter color value (HEX, HSL, RGB, RGBA)
ColorPicker->>+State: Update colorInputs state
State-->>-ColorPicker: colorInputs updated
ColorPicker->>User: Display updated color
Assessment against linked issues
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Great job, @harini-1597! 🎉 Thank you for submitting your pull request. Your contribution is valuable and we appreciate your efforts to improve our project. Keep being the part of the community!
'We will promptly review your changes and offer feedback. Keep up the excellent work!
Kindly remember to check our Contributing Guidelines'
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Outside diff range and nitpick comments (1)
src/components/core/colorPicker/index.tsx (1)
47-81: Review the layout and accessibility of the color picker component.Consider adding
aria-labelsto inputs for better accessibility and ensure the layout is responsive.Tools
Biome
[error] 51-58: Enforce to have the onClick mouse event with the onKeyUp, the onKeyDown, or the onKeyPress keyboard event. (lint/a11y/useKeyWithClickEvents)
Actions triggered using mouse events should have corresponding keyboard events to account for keyboard-only navigation.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- src/components/core/colorPicker/index.tsx (2 hunks)
Additional context used
Biome
src/components/core/colorPicker/index.tsx
[error] 51-58: Enforce to have the onClick mouse event with the onKeyUp, the onKeyDown, or the onKeyPress keyboard event. (lint/a11y/useKeyWithClickEvents)
Actions triggered using mouse events should have corresponding keyboard events to account for keyboard-only navigation.
Additional comments not posted (1)
src/components/core/colorPicker/index.tsx (1)
6-6: Ensure the index signature forcolorsis appropriate for your use case.This change allows dynamic access to color properties, which seems aligned with the new feature requirements.
|
Hi @harini-1597 please acknowledge the changes raised by code review AI as well as undo the commented part at the end of the page. |
Sanchitbajaj02
added
🏆 good first issue
|
Hi @harini-1597 please give your update on the features that I have requested. If you didn't acknowledge it in 48 hrs, unfortunately I have to close this PR |
|
Hey; addressed the changes and it still works perfectly; you can merge the PR successfully! |
There was a problem hiding this comment.
Actionable comments posted: 4
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- src/components/core/colorPicker/index.tsx (5 hunks)
Additional context used
Biome
src/components/core/colorPicker/index.tsx
[error] 52-60: JSX elements without children should be marked as self-closing. In JSX, it is valid for any element to be self-closing. (lint/style/useSelfClosingElements)
Unsafe fix: Use a SelfClosingElement instead
Additional comments not posted (1)
src/components/core/colorPicker/index.tsx (1)
48-83: Add keyboard accessibility for color selection.Actions triggered using mouse events should have corresponding keyboard events to account for keyboard-only navigation.
+ onKeyUp={(e) => { if (e.key === 'Enter') showPicker(index + 1); }}Likely invalid or redundant comment.
Tools
Biome
[error] 52-60: JSX elements without children should be marked as self-closing. In JSX, it is valid for any element to be self-closing. (lint/style/useSelfClosingElements)
Unsafe fix: Use a SelfClosingElement instead
| color02: string; | ||
| color03: string; | ||
| color04: string; | ||
| [key: string]: string; |
There was a problem hiding this comment.
Ensure the dynamic key type is properly validated.
The dynamic key in the colors type declaration allows any string as a key, which could lead to potential runtime errors if keys do not match expected patterns. Consider adding more specific validation or constraints on the keys used in this object.
| <div | ||
| key={color} | ||
| onClick={() => showPicker(index + 1)} | ||
| onKeyUp={(e) => { if (e.key === 'Enter') showPicker(index + 1); }} | ||
| className={`cursor-pointer w-full ${index === 0 ? 'rounded-tl-[0.8rem] rounded-bl-[0.8rem]' : ''} ${index === 3 ? 'rounded-tr-[0.8rem] rounded-br-[0.8rem]' : ''}`} | ||
| style={{ | ||
| backgroundColor: colors[color], | ||
| }} | ||
| ></div> |
There was a problem hiding this comment.
Use self-closing tags for empty JSX elements.
JSX elements without children should be marked as self-closing to improve readability and maintain consistency with JSX style guidelines.
- <div style={{ backgroundColor: colors[color] }}></div>
+ <div style={{ backgroundColor: colors[color] }} />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.
| <div | |
| key={color} | |
| onClick={() => showPicker(index + 1)} | |
| onKeyUp={(e) => { if (e.key === 'Enter') showPicker(index + 1); }} | |
| className={`cursor-pointer w-full ${index === 0 ? 'rounded-tl-[0.8rem] rounded-bl-[0.8rem]' : ''} ${index === 3 ? 'rounded-tr-[0.8rem] rounded-br-[0.8rem]' : ''}`} | |
| style={{ | |
| backgroundColor: colors[color], | |
| }} | |
| ></div> | |
| <div | |
| key={color} | |
| onClick={() => showPicker(index + 1)} | |
| onKeyUp={(e) => { if (e.key === 'Enter') showPicker(index + 1); }} | |
| className={`cursor-pointer w-full ${index === 0 ? 'rounded-tl-[0.8rem] rounded-bl-[0.8rem]' : ''} ${index === 3 ? 'rounded-tr-[0.8rem] rounded-br-[0.8rem]' : ''}`} | |
| style={{ | |
| backgroundColor: colors[color], | |
| }} | |
| /> |
Tools
Biome
[error] 52-60: JSX elements without children should be marked as self-closing. In JSX, it is valid for any element to be self-closing. (lint/style/useSelfClosingElements)
Unsafe fix: Use a SelfClosingElement instead
| const handleInputChange = (e: React.ChangeEvent<HTMLInputElement>, section: number) => { | ||
| const value = e.target.value; | ||
| setColorInputs({ ...colorInputs, [`color0${section}`]: value }); | ||
| if (/^#([0-9A-F]{3}){1,2}$/i.test(value)) { | ||
| changeHandler(value); | ||
| } |
There was a problem hiding this comment.
Validate HEX color codes more robustly in handleInputChange.
The current regex does not support 3-character HEX codes. Consider supporting both 3 and 6-character codes.
- if (/^#([0-9A-F]{3}){1,2}$/i.test(value)) {
+ if (/^#([0-9A-F]{3}){1,2}$/i.test(value)) {Committable suggestion was skipped due to low confidence.
| const [colorInputs, setColorInputs] = useState(colors); | ||
|
|
||
| const changeHandler = (color: string) => { | ||
| if (!isPalleteTouched) { | ||
| setTouched(true); | ||
| } | ||
| if (typeof color === "string") | ||
| setColors({ ...colors, [`color0` + selectedColorSection]: color }); | ||
| if (typeof color === "string") { | ||
| const updatedColors = { ...colors, [`color0${selectedColorSection}`]: color }; | ||
| setColors(updatedColors); | ||
| setColorInputs(updatedColors); | ||
| } |
There was a problem hiding this comment.
Refactor to simplify state management in changeHandler.
Consider combining setColors and setColorInputs into a single state update function to reduce redundancy and potential bugs.
- setColors({ ...colors, [`color0${selectedColorSection}`]: color });
- setColorInputs({ ...colorInputs, [`color0${selectedColorSection}`]: color });
+ const updatedColors = { ...colors, [`color0${selectedColorSection}`]: color };
+ setColors(updatedColors);
+ setColorInputs(updatedColors);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.
| const [colorInputs, setColorInputs] = useState(colors); | |
| const changeHandler = (color: string) => { | |
| if (!isPalleteTouched) { | |
| setTouched(true); | |
| } | |
| if (typeof color === "string") | |
| setColors({ ...colors, [`color0` + selectedColorSection]: color }); | |
| if (typeof color === "string") { | |
| const updatedColors = { ...colors, [`color0${selectedColorSection}`]: color }; | |
| setColors(updatedColors); | |
| setColorInputs(updatedColors); | |
| } | |
| const [colorInputs, setColorInputs] = useState(colors); | |
| const changeHandler = (color: string) => { | |
| if (!isPalleteTouched) { | |
| setTouched(true); | |
| } | |
| if (typeof color === "string") { | |
| const updatedColors = { ...colors, [`color0${selectedColorSection}`]: color }; | |
| setColors(updatedColors); | |
| setColorInputs(updatedColors); | |
| } |
Related Issue
fixes: #321
Description
The code adds a HEX color code box such that user can add specific colors to his palettes. It makes it easier instead of dragging pointer to find exact color.
The user selects the specific color box, then either selects a particular color from the Colorpicker or uses a hex color code in the text box given below
Screenshots
Recording.2024-06-06.141524.mp4
Summary by CodeRabbit
New Features
Refactor