Skip to content

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

Merged
merged 5 commits into from
Mar 27, 2025

Conversation

spartan-vutrannguyen
Copy link
Contributor

@spartan-vutrannguyen spartan-vutrannguyen commented Mar 27, 2025

Description

  • Fix error when add new color to group.
  • Create new variable in css file if it does not exist.
  • Adjust Color group name display

Related Issues

Type of Change

  • Bug fix
  • New feature
  • Documentation update
  • Release
  • Refactor
  • Other (please describe):

Testing

Screenshots (if applicable)

Create new variable in css file if it does not exist.

If I update a color in the left panel, then click out of it, it turns black, and then seems to assign a variable to the value that should be in the globals.css file, but there is no variable in the globals.css file.

Screen.Recording.2025-03-27.at.17.51.29.mp4
  • Fix error when add new color to group.
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.

  • Behavior:
    • Fix error when adding a new color to a group in styles.ts by ensuring CSS variables are created if they don't exist.
    • Adjust color group name display using toNormalCase() in ColorBrandPicker.tsx and ColorPalletGroup.tsx.
  • Functions:
    • Update updateTailwindColorConfig() and createTailwindColorVariable() in styles.ts to handle missing CSS variables.
    • Modify updateTailwindCssVariable() to create new variables if missing.
  • UI Components:
    • Use toNormalCase() for displaying color group names in ColorBrandPicker.tsx and ColorPalletGroup.tsx.
    • Add input validation for group names in ColorPalletGroup.tsx.
  • Misc:
    • Update ThemeManager in index.ts to handle color updates and renames more robustly.
    • Ensure consistent naming conventions using camelCase and toNormalCase across components.

This description was created by Ellipsis for 99f0238. It will automatically update as commits are pushed.

Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a 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 in 5 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 the parentName 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 how originalParentName is used, it's only passed to UPDATE_TAILWIND_CONFIG as the parentName 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%
  1. 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% <= threshold 50%
    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% <= threshold 50%
    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% <= threshold 50%
    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: using newClass ? ... : '' 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% <= threshold 50%
    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:
    Using camelCase(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% <= threshold 50%
    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% <= threshold 50%
    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.

Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a 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 in 2 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% <= threshold 50%
    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% <= threshold 50%
    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% <= threshold 50%
    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% <= threshold 50%
    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% <= threshold 50%
    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% <= threshold 50%
    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% <= threshold 50%
    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% <= threshold 50%
    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.

Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a 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 in 2 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% <= threshold 50%
    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% <= threshold 50%
    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% <= threshold 50%
    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% <= threshold 50%
    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.

@Kitenite Kitenite merged commit 0ab072b into main Mar 27, 2025
@Kitenite Kitenite deleted the fix/error-when-add-new-color-to-color-group branch March 27, 2025 23:48
zongkelong pushed a commit to zongkelong/coolook that referenced this pull request Mar 28, 2025
…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)
ml-delaurier pushed a commit to ml-delaurier/nolook that referenced this pull request Apr 23, 2025
* fix: error when add new color to group

* update variable in css file

* fix display in color picker
t1c1 pushed a commit to t1c1/onlookbotcodes that referenced this pull request Jun 5, 2025
* fix: error when add new color to group

* update variable in css file

* fix display in color picker
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants