-
Notifications
You must be signed in to change notification settings - Fork 595
add brand color picker #1873
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
add brand color picker #1873
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
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.
Caution
Changes requested ❌
Reviewed everything up to 5b76b72 in 2 minutes and 16 seconds. Click for details.
- Reviewed
302
lines of code in3
files - Skipped
0
files when reviewing. - Skipped posting
4
draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. apps/web/client/src/app/project/[id]/_components/editor-bar/inputs/color-picker.tsx:101
- Draft comment:
Pressing Enter in the search input clears the query. Verify that this behavior meets UX expectations. - Reason this comment was not posted:
Confidence changes required:50%
<= threshold50%
None
2. apps/web/client/src/components/store/editor/theme/index.ts:56
- Draft comment:
Calling async scanConfig() in the constructor without awaiting it may lead to race conditions. Consider initializing asynchronously or handling the promise properly. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50% The concern about race conditions is valid - scanConfig() updates observable state like brandColors and defaultColors asynchronously. Not handling the promise means those updates could happen at unpredictable times. However, looking at the code, this appears intentional - the class is designed to handle the async initialization and updates to state. The void operator suggestion would just suppress the warning without changing behavior. I could be wrong about the intentional design - maybe there are assumptions elsewhere in the code about when initialization completes. The void operator would at least make the intent explicit. The class appears to be designed for async state management with MobX observables. The current code is likely intentional rather than a mistake needing fixing. While technically correct about the async call, this appears to be a valid pattern for this class's async initialization. The comment and suggestion don't add value.
3. packages/parser/src/code-edit/insert.ts:94
- Draft comment:
Index clamping in insertAtIndex may incorrectly insert at the last valid element instead of appending when index equals the count of jsxElements. Consider checking if (index >= jsxElements.length) to append. - Reason this comment was not posted:
Comment was on unchanged code.
4. packages/parser/src/code-edit/insert.ts:38
- Draft comment:
When both codeBlock and pasteParams are provided, DATA_ONLOOK_ID may be added twice. Ensure duplicate attributes are handled as intended. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
Workflow ID: wflow_ylyMdIVbewIBtSlq
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
apps/web/client/src/app/project/[id]/_components/editor-bar/inputs/color-picker.tsx
Show resolved
Hide resolved
@@ -53,6 +53,7 @@ export class ThemeManager { | |||
private projectManager: ProjectManager, | |||
) { | |||
makeAutoObservable(this); | |||
this.scanConfig(); |
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.
We should probably wrap this in a reaction to session instead?
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.
We should probably also scanConfig when opening the color picker's brand tab
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.
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.
Caution
Changes requested ❌
Reviewed 5ddd766 in 1 minute and 52 seconds. Click for details.
- Reviewed
81
lines of code in2
files - Skipped
0
files when reviewing. - Skipped posting
5
draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. apps/web/client/src/app/project/[id]/_components/editor-bar/inputs/color-picker.tsx:5
- Draft comment:
Unused import: 'useMemo' is imported but never used. Please remove it. - Reason this comment was not posted:
Confidence changes required:33%
<= threshold50%
None
2. apps/web/client/src/app/project/[id]/_components/editor-bar/inputs/color-picker.tsx:101
- Draft comment:
The useEffect dependency uses the entire theme object. Consider narrowing the dependency to specific properties (e.g. colorGroups, colorDefaults) to avoid unnecessary updates. - Reason this comment was not posted:
Confidence changes required:33%
<= threshold50%
None
3. apps/web/client/src/app/project/[id]/_components/editor-bar/inputs/color-picker.tsx:139
- Draft comment:
Local 'theme' state is fixed to LIGHT and never updated; consider deriving it from context or props to properly support dark mode. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
4. apps/web/client/src/app/project/[id]/_components/editor-bar/inputs/color-picker.tsx:118
- Draft comment:
Filtering of color groups is done on every render. Consider memoizing the filtered results (using useMemo) for performance optimization if the data set grows. - Reason this comment was not posted:
Confidence changes required:33%
<= threshold50%
None
5. apps/web/client/src/components/store/editor/theme/index.ts:53
- Draft comment:
Removed scanConfig() from the ThemeManager constructor. Ensure that this side effect is intentionally moved to the component lifecycle. - Reason this comment was not posted:
Confidence changes required:33%
<= threshold50%
None
Workflow ID: wflow_Vm436wbxxTr0x2a2
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
|
||
useEffect(() => { | ||
setPalette(color.palette); | ||
}, [color]); | ||
|
||
useEffect(() => { | ||
editorEngine.theme.scanConfig(); |
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.
scanConfig()
is an async function called in useEffect
without error handling. Consider wrapping it in an async IIFE to catch potential errors.
5ddd766
to
6a1fe97
Compare
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.
Caution
Changes requested ❌
Reviewed 2448abc in 1 minute and 57 seconds. Click for details.
- Reviewed
41
lines of code in2
files - Skipped
0
files when reviewing. - Skipped posting
2
draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. apps/web/client/src/components/store/editor/theme/index.ts:720
- Draft comment:
Good cleanup: Removal of the debug console.log that printed config and css paths. - Reason this comment was not posted:
Confidence changes required:0%
<= threshold50%
None
2. apps/web/client/src/app/project/[id]/_components/editor-bar/inputs/color-picker.tsx:105
- Draft comment:
Typographical Issue: The error message 'Failed to scan fonts:' appears to be a misnomer since the function being called is scanConfig. Please update the message to accurately reflect the scanned data (e.g., 'Failed to scan config:'). - Reason this comment was not posted:
Marked as duplicate.
Workflow ID: wflow_Id1jj5HYz8Ef2H1M
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
useEffect(() => { | ||
(async () => { | ||
try { | ||
await editorEngine.theme.scanConfig().then(() => { |
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.
Avoid mixing await
and .then
. Instead, simply use await
to get the result and then update state.
apps/web/client/src/app/project/[id]/_components/editor-bar/inputs/color-picker.tsx
Show resolved
Hide resolved
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.
Important
Looks good to me! 👍
Reviewed f0609ff in 1 minute and 29 seconds. Click for details.
- Reviewed
60
lines of code in1
files - Skipped
0
files when reviewing. - Skipped posting
5
draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. apps/web/client/src/app/project/[id]/_components/editor-bar/inputs/color-picker.tsx:103
- Draft comment:
Update error message: 'Failed to scan fonts:' is misleading for a color/theme scan. - Reason this comment was not posted:
Comment was on unchanged code.
2. apps/web/client/src/app/project/[id]/_components/editor-bar/inputs/color-picker.tsx:269
- Draft comment:
Remove optional chaining on onChangeEnd; it's a required prop per the interface. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
3. apps/web/client/src/app/project/[id]/_components/editor-bar/inputs/color-picker.tsx:91
- Draft comment:
Theme is hardcoded to LIGHT. For proper dark mode support, consider deriving the theme from context or editor state. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
4. apps/web/client/src/app/project/[id]/_components/editor-bar/inputs/color-picker.tsx:97
- Draft comment:
Consider adding 'editorEngine' to the dependency array of this useEffect if it can change over time. - Reason this comment was not posted:
Comment was on unchanged code.
5. apps/web/client/src/app/project/[id]/_components/editor-bar/inputs/color-picker.tsx:104
- Draft comment:
Typographical note: The error message says 'Failed to scan fonts:' even though the function being called is scanConfig for the theme. Consider updating it to accurately describe the operation. - Reason this comment was not posted:
Comment was on unchanged code.
Workflow ID: wflow_tvmHEwMekQjz23F1
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
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.
Important
Looks good to me! 👍
Reviewed a53d03f in 1 minute and 41 seconds. Click for details.
- Reviewed
72
lines of code in1
files - Skipped
0
files when reviewing. - Skipped posting
4
draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. apps/web/client/src/app/project/[id]/_components/editor-bar/text-selected.tsx:12
- Draft comment:
Clean-up note: Duplicate imports for Popover/ColorPickerContent have been removed and reordered. Ensure the import order remains consistent across the file. - Reason this comment was not posted:
Confidence changes required:0%
<= threshold50%
None
2. apps/web/client/src/app/project/[id]/_components/editor-bar/text-selected.tsx:75
- Draft comment:
Styling suggestion: Instead of concatenating conditional CSS classes via template strings, consider using a utility like 'classnames' for better readability and maintainability. - Reason this comment was not posted:
Confidence changes required:33%
<= threshold50%
None
3. apps/web/client/src/app/project/[id]/_components/editor-bar/text-selected.tsx:288
- Draft comment:
Verify intent: Removal of the 'z-10' class from PopoverContent might affect the stacking context. Confirm that this change is intentional for proper layering. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 0% vs. threshold = 50% 1. This is a pure UI styling change - it only affects how elements stack visually. 2. The rules explicitly state not to comment on UI/styling changes. 3. The comment starts with "Verify intent" which is explicitly called out as not useful in the rules. 4. The rules say to assume UI changes were done correctly. The z-index could potentially affect functionality if this popover needs to appear above other elements. Maybe this is more than just styling? Even if it affects layering behavior, this is still fundamentally a UI concern. The rules clearly state to assume UI changes were done correctly by the author. This comment should be deleted as it violates multiple rules - it's about UI styling and asks for verification of intent.
4. apps/web/client/src/app/project/[id]/_components/editor-bar/text-selected.tsx:1
- Draft comment:
Note on scope: The PR title mentions a brand color picker and refactoring group/ungroup features, yet the linked issue (123) references onboarding docs. Ensure that all required changes (including documentation if needed) are included and aligned with the issue. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
Workflow ID: wflow_9wSJXRrLMGdkgp55
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
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.
Caution
Changes requested ❌
Reviewed 7f023f4 in 2 minutes and 15 seconds. Click for details.
- Reviewed
265
lines of code in7
files - Skipped
0
files when reviewing. - Skipped posting
2
draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. apps/web/client/src/app/project/[id]/_components/editor-bar/dropdowns/border.tsx:38
- Draft comment:
Removal of the direct style update (editorEngine.style.update) is intentional. Please ensure that using the InputColor component (with useColorUpdate hook) properly updates the border color. - Reason this comment was not posted:
Confidence changes required:20%
<= threshold50%
None
2. apps/web/client/src/app/project/[id]/_components/editor-bar/dropdowns/border.tsx:1
- Draft comment:
The PR description mentions adding onboarding documentation for various frameworks. No doc updates are visible in the diff. Please confirm if documentation and any group/ungroup refactors are included or planned in a separate PR. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
Workflow ID: wflow_ubvOt75ws7a1ML9D
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
|
||
const handleColorUpdate = useCallback( | ||
(newValue: Color | TailwindColor) => { | ||
console.log('handleColorUpdate', newValue); |
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.
Remove or disable the debug logging (console.log
) once debugging is complete to avoid unnecessary console output in production.
console.log('handleColorUpdate', newValue); |
} else { | ||
setTempColor(Color.from(newColor.lightColor)); | ||
} | ||
handleColorUpdate(tempColor); |
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.
In handleColorChangeEnd
, consider passing the newColor
parameter directly to handleColorUpdate
instead of using the state variable tempColor
. This avoids potential stale state issues.
handleColorUpdate(tempColor); | ||
} catch (error) { | ||
console.error('Error updating color:', error); | ||
} | ||
}, | ||
[onColorChange], |
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.
Include handleColorUpdate
in the dependency array of the useCallback
for handleColorChangeEnd
to ensure the callback reflects any changes in handleColorUpdate
.
[onColorChange], | |
[onColorChange, handleColorUpdate], |
7f023f4
to
d05b466
Compare
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.
Important
Looks good to me! 👍
Reviewed d05b466 in 52 seconds. Click for details.
- Reviewed
25
lines of code in2
files - Skipped
0
files when reviewing. - Skipped posting
1
draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. apps/web/client/src/app/project/[id]/_components/editor-bar/inputs/input-color.tsx:37
- Draft comment:
handleColorChangeEnd calls handleColorUpdate with 'tempColor', which may be stale because setTempColor is async. Consider computing the new color value (e.g. let newTemp = newColor instanceof Color ? newColor : Color.from(newColor.lightColor)) and then using that for both setTempColor and handleColorUpdate. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
Workflow ID: wflow_JxDRDkDBzjgFW4E8
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
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.
Caution
Changes requested ❌
Reviewed 7052439 in 1 minute and 39 seconds. Click for details.
- Reviewed
153
lines of code in3
files - Skipped
0
files when reviewing. - Skipped posting
2
draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. apps/web/client/src/app/project/[id]/_components/editor-bar/inputs/input-color.tsx:46
- Draft comment:
Wrap Color.from(value) in try/catch to handle invalid color input. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
2. apps/web/client/src/app/project/[id]/_components/editor-bar/dropdowns/border.tsx:1
- Draft comment:
PR description mismatch: Title/description mention onboarding docs, but changes implement color picker and refactoring. Update PR description accordingly. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
Workflow ID: wflow_fN0hn9rZE4kGNNBd
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
apps/web/client/src/app/project/[id]/_components/editor-bar/dropdowns/border.tsx
Outdated
Show resolved
Hide resolved
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.
Important
Looks good to me! 👍
Reviewed b5416d5 in 44 seconds. Click for details.
- Reviewed
15
lines of code in1
files - Skipped
0
files when reviewing. - Skipped posting
1
draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. apps/web/client/src/app/project/[id]/_components/editor-bar/dropdowns/border.tsx:26
- Draft comment:
Good removal of debugging artifacts: the console.log statement has been correctly removed. Ensure this aligns with the overall feature requirements for the brand color picker. - Reason this comment was not posted:
Confidence changes required:0%
<= threshold50%
None
Workflow ID: wflow_gZ5IU0IndJbX2uzk
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
* starting to design homepage * new sections, rotating contributor pic section * new sections, adjust topbar, footer * add contributor section component --------- Co-authored-by: Daniel R Farrell <[email protected]>
* update middleware * update fetch user error
* Add GitHub Actions workflows for Supabase staging and production environments * add migration files * update migration path --------- Co-authored-by: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com> Co-authored-by: [email protected] <[email protected]>
Description
Related Issues
Type of Change
Testing
Screenshots (if applicable)
Additional Notes
Important
Add brand color picker and refactor color handling with
useColorUpdate
hook across components.color-picker.tsx
with tabs for brand and custom colors.ColorGroup
component for displaying color groups.border.tsx
,color-background.tsx
,input-color.tsx
, andtypography.tsx
to useuseColorUpdate
hook.use-color-update.ts
to handle color updates with error handling.color-picker.tsx
for filtering color groups.text-selected.tsx
to includeTextColor
component for text color selection.insert.ts
for handling empty nodes ininsertAtIndex()
.This description was created by
for b5416d5. You can customize this summary. It will automatically update as commits are pushed.