refactor: update naming for brand color #1785

spartan-vutrannguyen · 2025-04-15T16:49:28Z

Description

Currently, generating a random name when creating a new color can give the impression of a bug due to inconsistent naming.
This PR updates the naming strategy to use a sequential counter pattern (e.g., Color 1, Color 2, Color 3, ...), ensuring predictable and user-friendly color names.

Related Issues

Fixes [bug] Brand panel isn't updated / refreshed between projects #1771

Type of Change

Testing

Screenshots (if applicable)

Additional Notes

Important

Refactor color naming strategy to use sequential counter pattern and introduce generateUniqueName utility function with tests.

Behavior:
- Updates color naming strategy to use sequential counter pattern (e.g., Color 1, Color 2) in ThemeManager and BrandPalletGroup.
- Introduces generateUniqueName function in string.ts to generate unique names by appending a number.
Functions:
- Adds generateUniqueName to string.ts for generating unique names with a counter.
Tests:
- Adds tests for generateUniqueName in string.test.ts to cover various naming scenarios.

^{This description was created by}^{for 64c27a6. It will automatically update as commits are pushed.}

ellipsis-dev

👍 Looks good to me! Reviewed everything up to 03438bc in 2 minutes and 12 seconds

More details

Looked at 189 lines of code in 4 files
Skipped 0 files when reviewing.
Skipped posting 11 drafted comments based on config settings.

1. apps/studio/src/lib/editor/engine/theme/index.ts:455

Draft comment:
Good use of generateUniqueName for non-numeric names to ensure consistent sequential naming.
Reason this comment was not posted:
Confidence changes required: 0% <= threshold 50%
None

2. apps/studio/src/lib/editor/engine/theme/index.ts:459

Draft comment:
Numeric names get a random numeric suffix; consider a sequential counter for consistency with non-numeric names.
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 comment points out an inconsistency in naming strategy between numeric and non-numeric names. However, there's no clear evidence that sequential numbering would be better than random suffixes for numeric names. The random suffix approach prevents collisions and may have been chosen deliberately. The current code works fine and the suggested change is more of a style preference.
The comment raises a valid point about consistency in naming patterns. Sequential numbering could make the naming more predictable and easier to understand.
While consistency is good, random suffixes have advantages like preventing collisions and the current code is working correctly. This seems more like a matter of preference than a clear improvement.
Delete the comment as it suggests a change that is more of a style preference than a clear improvement, and the current implementation is working correctly.

3. apps/studio/src/routes/editor/LayersPanel/BrandTab/ColorPanel/ColorPalletGroup.tsx:125

Draft comment:
generateUniqueColorName correctly leverages generateUniqueName. Ensure 'title' reliably represents the base name.
Reason this comment was not posted:
Confidence changes required: 0% <= threshold 50%
None

4. apps/studio/tests/string.test.ts:1

Draft comment:
Test cases for generateUniqueName are comprehensive and cover various edge cases.
Reason this comment was not posted:
Confidence changes required: 0% <= threshold 50%
None

5. packages/utility/src/string.ts:46

Draft comment:
generateUniqueName implementation is clear, relying on camelCase for consistent comparisons.
Reason this comment was not posted:
Confidence changes required: 0% <= threshold 50%
None

6. Overall:0

Draft comment:
PR description mentions onboarding docs but the changes focus solely on color naming refactor. Please update the PR description if necessary.
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.

7. apps/studio/src/lib/editor/engine/theme/index.ts:459

Draft comment:
For the duplicated color, non‐numeric names now use generateUniqueName to append a sequential counter. However, numeric base names still append a random numeric suffix via customAlphabet. To ensure a predictable sequential pattern (as described in the PR), consider using the same sequential strategy for numeric names.
Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 30% vs. threshold = 50%
The comment makes a reasonable point about consistency - having two different approaches for generating unique names could be confusing. However, there may be valid reasons for handling numeric names differently. Without understanding the full context of why numeric names use random suffixes, I can't be certain this suggestion is correct. The comment is also somewhat speculative in referencing "the PR" which I don't have access to.
I don't have visibility into why numeric names were implemented differently - there could be important technical reasons for this that the comment overlooks. The comment also assumes details about requirements from a PR that I can't verify.
While consistency is generally good, without understanding the full context and requirements, I can't be confident that forcing the same approach for both cases is the right solution.
This comment makes a reasonable suggestion but requires more context about the requirements and reasoning behind the current implementation to be truly actionable. Following the principle of removing uncertain comments, it should be deleted.

8. apps/studio/src/routes/editor/LayersPanel/BrandTab/ColorPanel/ColorPalletGroup.tsx:1

Draft comment:
The PR description mentions adding better onboarding docs, but the changes here focus solely on updating the color naming strategy. Please update the PR description to accurately reflect the actual changes.
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/src/lib/editor/engine/theme/index.ts:355

Draft comment:
Typo: In the comment at line 355, please change '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 not on a location in the diff, so it can't be submitted as a review comment.

10. apps/studio/src/routes/editor/LayersPanel/BrandTab/ColorPanel/ColorPalletGroup.tsx:1

Draft comment:
The file and component are named 'ColorPalletGroup', but since this group represents a set of colors (a 'palette'), consider renaming it to 'ColorPaletteGroup' to avoid confusion.
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.

11. apps/studio/src/routes/editor/LayersPanel/BrandTab/ColorPanel/ColorPalletGroup.tsx:73

Draft comment:
The variable 'existedName' (line 73) is used to store the list of existing color names. It would be more clear and idiomatic to rename it to 'existingNames'.
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_eY7T3XlXgj1KBX1D

You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

ellipsis-dev

Skipped PR review on 9f86cee because no changed files had a supported extension. If you think this was in error, please contact us and we'll fix it right away.

spartan-uyen · 2025-04-16T02:17:04Z

Title: Project 1 colors appear in Project 2 when Tailwind config is broken

Summary: When switching from a project with correctly configured Tailwind colors (Project 1) to another project with a broken Tailwind config (Project 2), the color palette from Project 1 appears in Project 2. Additionally, it's not possible to add or edit colors in Project 2.

Steps to Reproduce:

Open Project 1 where Tailwind config is working correctly and has defined colors.
Switch to Project 2 where the Tailwind config is broken or missing.
Open the color panel in Project 2.

Actual Result:
The colors from Project 1 appear in Project 2.
You cannot create, edit, or manage colors in Project 2.

Expected Result:
Project 2 should either show an empty or fallback color palette.
User should be able to add or edit colors independently in Project 2, regardless of Project 1’s state.

ellipsis-dev

❌ Changes requested. Incremental review on 9e14bdb in 1 minute and 10 seconds

More details

Looked at 36 lines of code in 1 files
Skipped 0 files when reviewing.
Skipped posting 7 drafted comments based on config settings.

1. apps/studio/src/lib/editor/engine/theme/index.ts:33

Draft comment:
Good extraction of state reset logic via the new reset() method. Consider adding a brief comment explaining its intent.
Reason this comment was not posted:
Confidence changes required: 0% <= threshold 50%
None

2. apps/studio/src/lib/editor/engine/theme/index.ts:41

Draft comment:
Resetting state when no projectRoot is found is a good safeguard.
Reason this comment was not posted:
Confidence changes required: 0% <= threshold 50%
None

3. apps/studio/src/lib/editor/engine/theme/index.ts:51

Draft comment:
Resetting state when configResult is falsy helps avoid stale state. Looks good.
Reason this comment was not posted:
Confidence changes required: 0% <= threshold 50%
None

4. apps/studio/src/lib/editor/engine/theme/index.ts:214

Draft comment:
Resetting state in the catch block is appropriate, ensuring consistency on error.
Reason this comment was not posted:
Confidence changes required: 0% <= threshold 50%
None

5. apps/studio/src/lib/editor/engine/theme/index.ts:41

Draft comment:
Note: The PR title and description mention a renaming refactor to use sequential counter names, but no changes related to renaming strategy are visible in this diff. Please ensure that the intended naming updates are included.
Reason this comment was not posted:
Comment did not seem useful. Confidence is useful = 0% <= threshold 50%
This comment is asking the PR author to ensure that the intended naming updates are included, which violates the rule against asking the author to ensure behavior is intended or to update the PR description. It does not provide a specific code suggestion or ask for a specific test to be written.

6. apps/studio/src/lib/editor/engine/theme/index.ts:41

Draft comment:
The diff adds reset() calls in error cases, which improves state consistency. Note: The PR description mentions updating to a sequential naming strategy for brand colors, but the changed code doesn’t reflect any change to naming logic.
Reason this comment was not posted:
Comment did not seem useful. Confidence is useful = 0% <= threshold 50%
The comment is pointing out a discrepancy between the PR description and the actual code changes. However, it is not asking for any specific action or providing a suggestion related to the code itself. It is more of an informative comment about the PR description, which violates the rules.

7. apps/studio/src/lib/editor/engine/theme/index.ts:3

Draft comment:
There appears to be a potential typographical error in the module path. The import statement uses 'ColorPalletGroup', but typically the word is spelled 'Palette'. Please double-check if this should be 'ColorPaletteGroup'.
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_lNPK2uBFXLExmhrI

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.

ellipsis-dev · 2025-04-16T02:58:04Z

apps/studio/src/lib/editor/engine/theme/index.ts

@@ -30,9 +30,15 @@ export class ThemeManager {
        this.scanConfig();
    }

+    private reset() {
+        this.defaultColors = {};


Consider also resetting configPath and cssPath in the reset() method to avoid retaining stale values.

ellipsis-dev

👍 Looks good to me! Incremental review on 64c27a6 in 31 seconds

More details

Looked at 38 lines of code in 1 files
Skipped 0 files when reviewing.
Skipped posting 7 drafted comments based on config settings.

1. apps/studio/src/lib/editor/engine/theme/index.ts:33

Draft comment:
Good use of a reset method to consistently clear state on config scan failures. Ensure that all necessary state is reset, but consider if additional properties should be cleared when reloading configuration.
Reason this comment was not posted:
Confidence changes required: 0% <= threshold 50%
None

2. apps/studio/src/lib/editor/engine/theme/index.ts:469

Draft comment:
The PR description specifies a sequential counter naming strategy (e.g. 'Color 1', 'Color 2', ...) for new brand colors, but this diff still uses generateUniqueName and customAlphabet. Update naming logic to match the sequential counter requirement.
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/studio/src/lib/editor/engine/theme/index.ts:33

Draft comment:
Good refactoring: the new reset() method centralizes clearing of theme state. Consider verifying that resetting configPath and cssPath here is intended in all error cases.
Reason this comment was not posted:
Confidence changes required: 0% <= threshold 50%
None

4. apps/studio/src/lib/editor/engine/theme/index.ts:42

Draft comment:
Nice addition: calling reset() when projectRoot is missing ensures state consistency. Confirm that this reset doesn’t mask potential underlying config issues.
Reason this comment was not posted:
Confidence changes required: 0% <= threshold 50%
None

5. apps/studio/src/lib/editor/engine/theme/index.ts:50

Draft comment:
Resetting the state when configResult is falsy is a good defensive measure.
Reason this comment was not posted:
Confidence changes required: 0% <= threshold 50%
None

6. apps/studio/src/lib/editor/engine/theme/index.ts:216

Draft comment:
Resetting state in the catch block for scanConfig helps maintain consistency after errors.
Reason this comment was not posted:
Confidence changes required: 0% <= threshold 50%
None

7. apps/studio/src/lib/editor/engine/theme/index.ts:466

Draft comment:
The PR description mentions updating brand color naming to a sequential counter, yet in duplicate() the logic still uses generateUniqueName (for non-numeric names) and a random numeric suffix for numeric names. Confirm if generateUniqueName now returns sequential names or if additional changes are needed here to implement a sequential counter.
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_8yQcSjbZzfegDpaW

You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

* refactor: update naming for brand color * change location of test file * reset color if errors

* 'main' of https://github.com/onlook-dev/onlook: use DomElementStyles (onlook-dev#1810) feat: Added file watcher & auto update modified opened files in Dev Tab (onlook-dev#1802) fix: border thickness hidden when setting border color from color picker (onlook-dev#1805) move overflow input (onlook-dev#1800) Added overflow option in the style panel (onlook-dev#1793) refactor: update images manager (onlook-dev#1784) chore: error nesting button (onlook-dev#1790) refactor: update naming for brand color (onlook-dev#1785) Tys toolbar v4 (onlook-dev#1786)

* refactor: update naming for brand color * change location of test file * reset color if errors

refactor: update naming for brand color

03438bc

ellipsis-dev bot reviewed Apr 15, 2025

View reviewed changes

change location of test file

9f86cee

ellipsis-dev bot reviewed Apr 16, 2025

View reviewed changes

reset color if errors

64c27a6

spartan-vutrannguyen force-pushed the refactor/update-naming-for-brand-color branch from 9e14bdb to 64c27a6 Compare April 16, 2025 03:04

ellipsis-dev bot reviewed Apr 16, 2025

View reviewed changes

spartan-vutrannguyen requested a review from Kitenite April 16, 2025 03:07

Kitenite merged commit f82fc34 into main Apr 16, 2025

Kitenite deleted the refactor/update-naming-for-brand-color branch April 16, 2025 06:13

ml-delaurier pushed a commit to ml-delaurier/nolook that referenced this pull request Apr 23, 2025

refactor: update naming for brand color (onlook-dev#1785)

ed8212b

* refactor: update naming for brand color * change location of test file * reset color if errors

t1c1 pushed a commit to t1c1/onlookbotcodes that referenced this pull request Jun 5, 2025

refactor: update naming for brand color (onlook-dev#1785)

dd383db

* refactor: update naming for brand color * change location of test file * reset color if errors

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

refactor: update naming for brand color #1785

refactor: update naming for brand color #1785

Uh oh!

spartan-vutrannguyen commented Apr 15, 2025 •

edited

Loading

Uh oh!

ellipsis-dev bot left a comment

Uh oh!

ellipsis-dev bot left a comment

Uh oh!

spartan-uyen commented Apr 16, 2025

Uh oh!

ellipsis-dev bot left a comment

Uh oh!

ellipsis-dev bot Apr 16, 2025

Uh oh!

ellipsis-dev bot left a comment

Uh oh!

Uh oh!

refactor: update naming for brand color #1785

refactor: update naming for brand color #1785

Uh oh!

Conversation

spartan-vutrannguyen commented Apr 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Related Issues

Type of Change

Testing

Screenshots (if applicable)

Additional Notes

Uh oh!

ellipsis-dev bot left a comment

Choose a reason for hiding this comment

Uh oh!

ellipsis-dev bot left a comment

Choose a reason for hiding this comment

Uh oh!

spartan-uyen commented Apr 16, 2025

Uh oh!

ellipsis-dev bot left a comment

Choose a reason for hiding this comment

Uh oh!

ellipsis-dev bot Apr 16, 2025

Choose a reason for hiding this comment

Uh oh!

ellipsis-dev bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

spartan-vutrannguyen commented Apr 15, 2025 •

edited

Loading