-
Notifications
You must be signed in to change notification settings - Fork 1.1k
fix: error when add new color to group #1683
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
Conversation
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.
❌ Changes requested. Reviewed everything up to 67511ff in 2 minutes and 21 seconds
More details
- Looked at
331
lines of code in5
files - Skipped
0
files when reviewing. - Skipped posting
18
drafted comments based on config settings.
1. apps/studio/electron/main/assets/styles.ts:420
- Draft comment:
When appending new CSS variables for :root and .dark rules, ensure no duplicate declarations occur on repeated calls. Consider checking if the variable already exists before appending. - Reason this comment was not posted:
Comment looked like it was already resolved.
2. apps/studio/src/lib/editor/engine/theme/index.ts:348
- Draft comment:
Calling camelCase on parentName without checking for undefined may result in an empty string. Consider adding a null/undefined check before applying camelCase. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50%
Looking at the lodash docs,camelCase(undefined)
returns an empty string, which is actually the desired behavior here since we want an empty string for theparentName
parameter when it's not provided. The current code will work correctly even without the null check. The comment is suggesting a more verbose solution that doesn't actually fix any real problem.
I could be wrong about whether an empty string is actually the desired behavior here. Maybe there's some case where we need to distinguish between undefined and empty string.
Looking at howoriginalParentName
is used, it's only passed toUPDATE_TAILWIND_CONFIG
as theparentName
parameter. An empty string is a reasonable default here and matches the behavior elsewhere in the code.
Delete this comment. The current code works correctly since lodash's camelCase handles undefined gracefully by returning an empty string, which is the desired behavior.
3. apps/studio/src/lib/editor/engine/theme/index.ts:299
- Draft comment:
Passing originalKey as oldName without lower-casing (changed from previous behavior) might affect consistency with other parts of the system. Please verify the key format across updates. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50%
- The code previously lowercased these values, but that was removed. 2. Looking at the full file, I see camelCase being used in other places for consistency. 3. Without seeing the MainChannels.UPDATE_TAILWIND_CONFIG implementation, I can't be certain what format it expects. 4. The comment is speculative ("might affect") rather than definitive.
I don't have visibility into the MainChannels.UPDATE_TAILWIND_CONFIG implementation or other parts of the system that might depend on case sensitivity. The author may have had good reasons to remove toLowerCase().
Without clear evidence that lowercase is required, and given that the codebase seems to be moving toward preserving case (with camelCase usage), I should err on the side of trusting the author's change.
Delete the comment. It's speculative and lacks strong evidence that lowercase is required. The change appears intentional given other case-preserving changes in the codebase.
4. apps/studio/src/routes/editor/LayersPanel/BrandTab/ColorPanel/ColorPalletGroup.tsx:145
- Draft comment:
Renaming now uses camelCase for group names. Consider whether the UI should display the normalized value or retain original casing for user friendliness. - Reason this comment was not posted:
Confidence changes required:50%
<= threshold50%
None
5. apps/studio/src/routes/editor/LayersPanel/BrandTab/ColorPanel/ColorPopover.tsx:91
- Draft comment:
The disabled attribute now checks if brandColor equals DEFAULT_COLOR_NAME. Ensure this reflects the intended UX and that non-editable default colors are clearly communicated to users. - Reason this comment was not posted:
Confidence changes required:40%
<= threshold50%
None
6. apps/studio/src/routes/editor/LayersPanel/BrandTab/ColorPanel/index.tsx:53
- Draft comment:
A debug console.log is present in handleColorChangeEnd; remove or adjust logging before production if not necessary. - Reason this comment was not posted:
Confidence changes required:50%
<= threshold50%
None
7. apps/studio/electron/main/assets/styles.ts:419
- Draft comment:
Consider adding an inline comment explaining why new CSS variables are appended when missing. This improves maintainability. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50%
The code is fairly self-documenting - the variable names hasRootVar/hasDarkVar and the if conditions make it clear what's happening. The suggested comment doesn't add significant new information beyond what's already obvious from the code. The existing comment on line 420 already provides adequate context.
The code deals with CSS variable management which can be complex, so additional documentation could help future maintainers understand the system better.
While CSS variable management can be complex, this specific block is straightforward and already has a clear comment. The suggested comment would be redundant.
The comment should be deleted as it suggests adding redundant documentation to already clear and commented code.
8. apps/studio/electron/main/assets/styles.ts:438
- Draft comment:
Good safeguard: usingnewClass ? ... : ''
avoids malformed class strings when newClass is empty. - 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.
9. apps/studio/electron/main/assets/styles.ts:782
- Draft comment:
Deletion now correctly clears class references by replacing with an empty string; ensure this covers all edge cases. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%
<= threshold50%
The comment is asking the PR author to ensure that all edge cases are covered, which is a form of asking them to double-check their work. This violates the rule against asking the author to ensure behavior is intended or tested.
10. apps/studio/src/lib/editor/engine/theme/index.ts:299
- Draft comment:
Removing .toLowerCase() conversion in invokeMainChannel calls ensures consistency with camelCase keys. Confirm that downstream consumers expect camelCase. - Reason this comment was not posted:
Comment looked like it was already resolved.
11. apps/studio/src/lib/editor/engine/theme/index.ts:348
- Draft comment:
UsingcamelCase(parentName)
for originalParentName is concise, but ensure that undefined parent names yield an empty string as intended. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%
<= threshold50%
The comment is asking to ensure a specific behavior when using a function. It seems to be asking for a confirmation of intended behavior, which is against the rules. It doesn't provide a specific code suggestion or ask for a test to be written.
12. apps/studio/src/routes/editor/LayersPanel/BrandTab/ColorPanel/ColorPalletGroup.tsx:106
- Draft comment:
Using toNormalCase(title) for renaming improves display consistency. Verify that validation (regex and uniqueness) remains in sync with this format. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%
<= threshold50%
The comment is asking the PR author to verify that validation remains in sync with a specific format. This falls under the category of asking the author to double-check or ensure something, which is against the rules.
13. apps/studio/src/routes/editor/LayersPanel/BrandTab/ColorPanel/ColorPopover.tsx:91
- Draft comment:
Disabling renaming when brandColor equals DEFAULT_COLOR_NAME is appropriate. Consider extracting this condition into a constant for clarity. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50%
The condition is straightforward and self-documenting. Creating a constant for a one-time use condition that's already clear would add unnecessary complexity. The condition is part of a simple disabled prop check alongside isDefaultPalette. The meaning is obvious from the constant name DEFAULT_COLOR_NAME.
Perhaps extracting the condition could help if this logic is likely to be reused elsewhere in the future. Maybe the condition's meaning isn't as obvious to others as it seems to me.
The condition is simple enough that extracting it would be overengineering. If the logic needs to be reused in the future, that would be the right time to extract it.
This comment should be deleted as it suggests an unnecessary refactor that wouldn't meaningfully improve code clarity or maintainability.
14. apps/studio/src/routes/editor/LayersPanel/BrandTab/ColorPanel/index.tsx:117
- Draft comment:
Passing groupName directly for display (instead of manually capitalizing) relies on upstream normalization. Verify user-facing names are still friendly. - 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.
15. apps/studio/electron/main/assets/styles.ts:864
- Draft comment:
Typo: In the comment that starts 'Update the specific shade base on tailwinds color scale', consider changing it to 'Update the specific shade based on Tailwind's color scale' for clarity. - 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.
16. apps/studio/src/lib/editor/engine/theme/index.ts:353
- Draft comment:
Typo: Consider changing 'Base on the class name, find the styles to update' to 'Based on the class name, find the styles to update'. - Reason this comment was not posted:
Comment was on unchanged code.
17. apps/studio/src/routes/editor/LayersPanel/BrandTab/ColorPanel/ColorPalletGroup.tsx:1
- Draft comment:
The filename and component name 'ColorPalletGroup' (and 'BrandPalletGroup') appear to use 'Pallet' which might be a typographical error given that it represents a group of colors (usually 'palette'). Please double-check if this was intentional or if it should be renamed to 'ColorPaletteGroup' for clarity. - 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.
18. apps/studio/src/routes/editor/LayersPanel/BrandTab/ColorPanel/index.tsx:9
- Draft comment:
There appears to be an inconsistency in the naming between 'ColorPalletGroup' (in the import path and component name 'BrandPalletGroup') and the comments/labels in the file (which use 'Palette'). Consider standardizing on one spelling (likely 'Palette') to improve clarity. - 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_nOWEB8n4iQNTU5Ap
Want Ellipsis to fix these issues? Tag @ellipsis-dev
in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
apps/studio/src/routes/editor/LayersPanel/BrandTab/ColorPanel/index.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.
❌ Changes requested. Incremental review on 0ed78f6 in 1 minute and 26 seconds
More details
- Looked at
108
lines of code in2
files - Skipped
0
files when reviewing. - Skipped posting
9
drafted comments based on config settings.
1. apps/studio/electron/main/assets/styles.ts:59
- Draft comment:
Remove debug log if not needed in production. - Reason this comment was not posted:
Confidence changes required:33%
<= threshold50%
None
2. apps/studio/electron/main/assets/styles.ts:243
- Draft comment:
Ensure that direct CSS variable values in StringLiteral nodes are updated correctly. - Reason this comment was not posted:
Confidence changes required:0%
<= threshold50%
None
3. apps/studio/electron/main/assets/styles.ts:488
- Draft comment:
Good addition: updating variable usages in @apply at-rules. Verify the regex covers all expected cases. - Reason this comment was not posted:
Confidence changes required:33%
<= threshold50%
None
4. apps/studio/src/routes/editor/LayersPanel/BrandTab/ColorPanel/index.tsx:50
- Draft comment:
Removal of the debug console.log in the color change end handler is appropriate. - Reason this comment was not posted:
Confidence changes required:0%
<= threshold50%
None
5. apps/studio/electron/main/assets/styles.ts:242
- Draft comment:
Added branch to handle StringLiteral values for child CSS variables. Ensure tests cover this scenario. - Reason this comment was not posted:
Confidence changes required:33%
<= threshold50%
None
6. apps/studio/electron/main/assets/styles.ts:483
- Draft comment:
Good addition handling variable usages in declaration values; verify regex correctness for edge cases. - Reason this comment was not posted:
Confidence changes required:33%
<= threshold50%
None
7. apps/studio/electron/main/assets/styles.ts:517
- Draft comment:
Review the regex in the '@apply' rule updater to ensure only intended class names are modified. - Reason this comment was not posted:
Confidence changes required:33%
<= threshold50%
None
8. apps/studio/src/routes/editor/LayersPanel/BrandTab/ColorPanel/index.tsx:50
- Draft comment:
Removal of the debug log in handleColorChangeEnd looks appropriate. - Reason this comment was not posted:
Confidence changes required:0%
<= threshold50%
None
9. apps/studio/src/routes/editor/LayersPanel/BrandTab/ColorPanel/index.tsx:9
- Draft comment:
Typo alert: The component is imported as 'BrandPalletGroup' from './ColorPalletGroup'. Consider verifying if 'Pallet' should be 'Palette' for consistency and clarity. - 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_666K9cAulIlX4NNg
Want Ellipsis to fix these issues? Tag @ellipsis-dev
in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and 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.
👍 Looks good to me! Incremental review on 99f0238 in 57 seconds
More details
- Looked at
34
lines of code in2
files - Skipped
0
files when reviewing. - Skipped posting
6
drafted comments based on config settings.
1. apps/studio/electron/main/assets/styles.ts:56
- Draft comment:
Good removal of the debug log. It's best not to leave console output in production. - Reason this comment was not posted:
Confidence changes required:0%
<= threshold50%
None
2. apps/studio/src/routes/editor/EditPanel/StylesTab/single/ColorInput/ColorBrandPicker.tsx:49
- Draft comment:
Nice update using toNormalCase to format the color group name for display. - Reason this comment was not posted:
Confidence changes required:0%
<= threshold50%
None
3. apps/studio/electron/main/assets/styles.ts:56
- Draft comment:
Removed the debug console.log for 'camelCaseName'. Good to clean up extra logging in production. If logging is needed later, consider a logging framework with proper log levels. - Reason this comment was not posted:
Confidence changes required:0%
<= threshold50%
None
4. apps/studio/src/routes/editor/EditPanel/StylesTab/single/ColorInput/ColorBrandPicker.tsx:49
- Draft comment:
Using toNormalCase(name) for displaying color group names improves consistency in the UI. Ensure that the transformation meets all branding requirements. - Reason this comment was not posted:
Confidence changes required:0%
<= threshold50%
None
5. apps/studio/src/routes/editor/EditPanel/StylesTab/single/ColorInput/ColorBrandPicker.tsx:3
- Draft comment:
It looks like 'ColorPalletGroup' might be a typo. If the intended word is 'Palette', please update the import path and any corresponding filenames/exports 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.
6. apps/studio/src/routes/editor/EditPanel/StylesTab/single/ColorInput/ColorBrandPicker.tsx:48
- Draft comment:
There is an extra space in the className string on line 48 ('gap-1 flex-1'). While this is cosmetic, it would be nice to have consistent spacing. - Reason this comment was not posted:
Comment was on unchanged code.
Workflow ID: wflow_9PwL2DG1o7xhDY6u
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
…slation_zh * 'main' of https://github.com/onlook-dev/onlook: fix: error when add new color to group (onlook-dev#1683) update searching brand color picker (onlook-dev#1681) Quicker build with freestyle (onlook-dev#1685) Add tool result support (onlook-dev#1684) feat: Include tool-result in chat (onlook-dev#1682)
* fix: error when add new color to group * update variable in css file * fix display in color picker
* fix: error when add new color to group * update variable in css file * fix display in color picker
Description
Related Issues
Type of Change
Testing
Screenshots (if applicable)
Create new variable in css file if it does not exist.
Screen.Recording.2025-03-27.at.17.51.29.mp4
Screen.Recording.2025-03-27.at.17.57.50.mp4
Additional Notes
Important
Fixes error when adding new color to group by ensuring CSS variables are created if missing and adjusts color group name display.
styles.ts
by ensuring CSS variables are created if they don't exist.toNormalCase()
inColorBrandPicker.tsx
andColorPalletGroup.tsx
.updateTailwindColorConfig()
andcreateTailwindColorVariable()
instyles.ts
to handle missing CSS variables.updateTailwindCssVariable()
to create new variables if missing.toNormalCase()
for displaying color group names inColorBrandPicker.tsx
andColorPalletGroup.tsx
.ColorPalletGroup.tsx
.ThemeManager
inindex.ts
to handle color updates and renames more robustly.camelCase
andtoNormalCase
across components.This description was created by
for 99f0238. It will automatically update as commits are pushed.