-
Notifications
You must be signed in to change notification settings - Fork 548
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
fix: issue with default brand color #1678
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.
👍 Looks good to me! Reviewed everything up to 5efc4ee in 46 seconds
More details
- Looked at
54
lines of code in3
files - Skipped
0
files when reviewing. - Skipped posting
8
drafted comments based on config settings.
1. apps/studio/electron/main/assets/styles.ts:57
- Draft comment:
Ensure 'DEFAULT' check is case-sensitive as intended. - Reason this comment was not posted:
Confidence changes required:0%
<= threshold50%
None
2. apps/studio/src/routes/editor/LayersPanel/BrandTab/ColorPanel/ColorPopover.tsx:36
- Draft comment:
Consistently preserve 'DEFAULT' by bypassing camelCase; verify this behavior meets design requirements. - Reason this comment was not posted:
Confidence changes required:0%
<= threshold50%
None
3. apps/studio/src/routes/editor/LayersPanel/BrandTab/ColorPanel/ColorPopover.tsx:86
- Draft comment:
AutoFocus added to input; confirm it aligns with overall focus management in the UI. - Reason this comment was not posted:
Confidence changes required:33%
<= threshold50%
None
4. packages/utility/src/string.ts:19
- Draft comment:
Bypassing conversion for fully uppercase strings (like 'DEFAULT') is clear; ensure this doesn't affect other acronyms. - Reason this comment was not posted:
Confidence changes required:0%
<= threshold50%
None
5. apps/studio/electron/main/assets/styles.ts:57
- Draft comment:
Bypass camelCase conversion for 'DEFAULT'. Consider a case-insensitive check if other variants (e.g. 'default') might occur. - Reason this comment was not posted:
Confidence changes required:0%
<= threshold50%
None
6. apps/studio/src/routes/editor/LayersPanel/BrandTab/ColorPanel/ColorPopover.tsx:36
- Draft comment:
Consistently bypass camelCase conversion for 'DEFAULT' to match the logic in styles.ts. - Reason this comment was not posted:
Confidence changes required:0%
<= threshold50%
None
7. apps/studio/src/routes/editor/LayersPanel/BrandTab/ColorPanel/ColorPopover.tsx:86
- Draft comment:
AutoFocus added to the input improves UX, but ensure it doesn't conflict when the input is disabled for default colors. - Reason this comment was not posted:
Confidence changes required:0%
<= threshold50%
None
8. packages/utility/src/string.ts:19
- Draft comment:
The new check for fully uppercase strings ensures that strings like 'DEFAULT' are not modified, aligning with the intended behavior. - Reason this comment was not posted:
Confidence changes required:0%
<= threshold50%
None
Workflow ID: wflow_KzzpLUww7hrqVDh4
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 5388c7c in 1 minute and 29 seconds
More details
- Looked at
244
lines of code in7
files - Skipped
0
files when reviewing. - Skipped posting
19
drafted comments based on config settings.
1. apps/studio/electron/main/assets/helpers.ts:126
- Draft comment:
Good: Replaced hard-coded 'DEFAULT' with DEFAULT_COLOR_NAME for consistency. - Reason this comment was not posted:
Confidence changes required:0%
<= threshold50%
None
2. apps/studio/electron/main/assets/styles.ts:58
- Draft comment:
Nice: Using DEFAULT_COLOR_NAME to decide camelCase conversion improves maintainability. - Reason this comment was not posted:
Confidence changes required:0%
<= threshold50%
None
3. apps/studio/electron/main/assets/styles.ts:230
- Draft comment:
Ensure consistency: Handling for DEFAULT color now uses DEFAULT_COLOR_NAME consistently. - Reason this comment was not posted:
Confidence changes required:0%
<= threshold50%
None
4. apps/studio/src/lib/editor/engine/theme/index.ts:77
- Draft comment:
Good refactor: Replacing 'DEFAULT' with DEFAULT_COLOR_NAME when checking object properties. - Reason this comment was not posted:
Confidence changes required:0%
<= threshold50%
None
5. apps/studio/src/routes/editor/EditPanel/StylesTab/single/ColorInput/index.tsx:129
- Draft comment:
Good: Replacing literal 'DEFAULT' with DEFAULT_COLOR_NAME in string operations. - Reason this comment was not posted:
Confidence changes required:0%
<= threshold50%
None
6. apps/studio/src/routes/editor/LayersPanel/BrandTab/ColorPanel/ColorPalletGroup.tsx:187
- Draft comment:
Note: Changed text color class from 'text-muted-foreground' to 'text-background-tertiary'. Verify design intent. - Reason this comment was not posted:
Confidence changes required:0%
<= threshold50%
None
7. apps/studio/src/routes/editor/LayersPanel/BrandTab/ColorPanel/ColorPopover.tsx:39
- Draft comment:
Good: Using DEFAULT_COLOR_NAME constant in place of hard-coded 'DEFAULT' improves consistency. - Reason this comment was not posted:
Confidence changes required:0%
<= threshold50%
None
8. packages/models/src/constants/editor.ts:90
- Draft comment:
Excellent: Added export for DEFAULT_COLOR_NAME. Ensure all modules reference this constant. - Reason this comment was not posted:
Confidence changes required:0%
<= threshold50%
None
9. apps/studio/electron/main/assets/helpers.ts:125
- Draft comment:
Good: Replaced literal 'DEFAULT' with DEFAULT_COLOR_NAME in the object property key for consistency. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%
<= threshold50%
This comment is purely informative, as it only describes a change that was made without providing any suggestion, question, or request for confirmation. It doesn't align with the rules for useful comments.
10. apps/studio/electron/main/assets/styles.ts:58
- Draft comment:
Nice use of DEFAULT_COLOR_NAME for comparing newName when generating camelCaseName; this avoids hard-coded strings. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%
<= threshold50%
This comment is purely informative, as it praises the use ofDEFAULT_COLOR_NAME
without providing any actionable feedback or suggestions for improvement. It does not align with the rules for useful comments.
11. apps/studio/src/lib/editor/engine/theme/index.ts:185
- Draft comment:
Consider using the DEFAULT_COLOR_NAME constant when constructing the originalKey. For example, instead of${key}-DEFAULT
, use${key}-${DEFAULT_COLOR_NAME}
for full consistency. - Reason this comment was not posted:
Comment was on unchanged code.
12. apps/studio/src/routes/editor/EditPanel/StylesTab/single/ColorInput/index.tsx:129
- Draft comment:
Good update: Using DEFAULT_COLOR_NAME in the endsWith check ensures that default color handling is consistent. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%
<= threshold50%
This comment is purely informative and does not provide any actionable feedback or suggestions for improvement. It simply states what the code is doing without offering any critique or suggestion for change.
13. apps/studio/src/routes/editor/LayersPanel/BrandTab/ColorPanel/ColorPopover.tsx:39
- Draft comment:
Nice: Replacing the hard-coded 'DEFAULT' with DEFAULT_COLOR_NAME (and using camelCase accordingly) ensures uniform default color reference. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%
<= threshold50%
This comment is purely informative, as it praises a change without providing any actionable feedback or suggestions. It doesn't ask for confirmation or suggest improvements, so it should be removed according to the rules.
14. packages/models/src/constants/editor.ts:90
- Draft comment:
DEFAULT_COLOR_NAME is exported correctly, which provides a single source of truth for default color naming. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%
<= threshold50%
This comment is purely informative and does not provide any actionable feedback or suggestions for improvement. It simply states that something is done correctly, which does not align with the rules for useful comments.
15. apps/studio/src/lib/editor/engine/theme/index.ts:3
- Draft comment:
Minor typographical error: The module name 'ColorPalletGroup' might be a typo. If this is meant to refer to a color palette grouping, consider renaming it to 'ColorPaletteGroup' for clarity and consistency. - Reason this comment was not posted:
Comment was on unchanged code.
16. apps/studio/src/routes/editor/EditPanel/StylesTab/single/ColorInput/index.tsx:4
- Draft comment:
Typographical note: Verify whether the imported type should be 'ColorPaletteGroup' instead of 'ColorPalletGroup'. If this import is intentional, please disregard this comment. - 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 component and file names use 'Pallet' (e.g. BrandPalletGroup, ColorPalletGroup). Since this relates to choosing colors, you might have intended to use 'Palette' (as in 'color palette'). Consider renaming for clarity. - Reason this comment was not posted:
Comment was on unchanged code.
18. apps/studio/src/routes/editor/LayersPanel/BrandTab/ColorPanel/ColorPalletGroup.tsx:74
- Draft comment:
The variable 'existedName' is used to store the list of color names. Consider renaming it to 'existingNames' (or something similar) for improved clarity and consistency. - 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.
19. packages/models/src/constants/editor.ts:48
- Draft comment:
Typographical error: the member 'Potrait' in the Orientation enum appears to be misspelled. Consider changing it to 'Portrait' for clarity and consistency. - 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_z3MjErGtm6cuaWBE
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 63e7ca6 in 59 seconds
More details
- Looked at
13
lines of code in1
files - Skipped
0
files when reviewing. - Skipped posting
5
drafted comments based on config settings.
1. apps/studio/electron/main/assets/styles.ts:54
- Draft comment:
The removal of the '// Skip if name is default' comment might be confusing. Consider replacing it with a comment that clarifies the logic: if newName equals DEFAULT_COLOR_NAME, it remains unchanged; otherwise, it's converted to camelCase. - Reason this comment was not posted:
Confidence changes required:33%
<= threshold50%
None
2. apps/studio/electron/main/assets/styles.ts:54
- Draft comment:
The removed comment (‘Skip if name is default’) isn’t needed since the logic on the next line correctly avoids camelCase conversion if newName equals DEFAULT_COLOR_NAME. However, consider adding a clarifying comment explaining that default brand colors must retain their naming convention. - Reason this comment was not posted:
Confidence changes required:0%
<= threshold50%
None
3. apps/studio/electron/main/assets/styles.ts:54
- Draft comment:
Note: The PR title and description reference different issues (default brand color fix vs. onboarding docs). Please ensure the PR metadata aligns with the actual code changes. - Reason this comment was not posted:
Confidence changes required:0%
<= threshold50%
None
4. apps/studio/electron/main/assets/styles.ts:836
- Draft comment:
Consider changing the comment from "Update the specific shade base on tailwinds color scale" to "Update the specific shade based on Tailwind's color scale" for better clarity and correctness. - 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.
5. apps/studio/electron/main/assets/styles.ts:468
- Draft comment:
Consider rephrasing the comment "Handle nested variables rename if existed" to something like "Handle nested variable renames if they exist" for improved grammar. - 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_sCgiGy5jKpOtnIsI
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 271a720 in 40 seconds
More details
- Looked at
58
lines of code in2
files - Skipped
0
files when reviewing. - Skipped posting
5
drafted comments based on config settings.
1. apps/studio/src/routes/editor/EditPanel/StylesTab/single/ColorInput/index.tsx:10
- Draft comment:
Remove unused commented-out PopoverPicker code if not needed. - Reason this comment was not posted:
Confidence changes required:33%
<= threshold50%
None
2. apps/studio/src/routes/editor/LayersPanel/index.tsx:175
- Draft comment:
Removal of 'hidden' from BRAND tab button now makes it visible – verify intentional visibility change. - Reason this comment was not posted:
Confidence changes required:0%
<= threshold50%
None
3. apps/studio/src/routes/editor/EditPanel/StylesTab/single/ColorInput/index.tsx:10
- Draft comment:
Updated import to use BrandPopoverPicker. Consider removing the unused PopoverPicker import (and its commented code below) if they are no longer needed. - Reason this comment was not posted:
Confidence changes required:33%
<= threshold50%
None
4. apps/studio/src/routes/editor/EditPanel/StylesTab/single/ColorInput/index.tsx:210
- Draft comment:
Remove legacy commented-out JSX for PopoverPicker if it’s no longer used to improve code clarity. - Reason this comment was not posted:
Confidence changes required:33%
<= threshold50%
None
5. apps/studio/src/routes/editor/LayersPanel/index.tsx:159
- Draft comment:
Removed the 'hidden' CSS class from the Brand tab button to make it visible. Confirm that this aligns with the design requirements for showing the default brand color. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =40%
<= threshold50%
The comment is asking the PR author to confirm that the removal of the 'hidden' CSS class aligns with design requirements. This is a request for confirmation, which violates the rule against asking the author to confirm their intention. However, it does point out a specific change that could have implications, which might be useful for the author to consider.
Workflow ID: wflow_77ED2rZFFT87CQvS
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
Description
Related Issues
Type of Change
Testing
Screenshots (if applicable)
Additional Notes
Important
Fixes default brand color issue by replacing hardcoded 'DEFAULT' with
DEFAULT_COLOR_NAME
across color-related functions and components.DEFAULT_COLOR_NAME
in color-related functions and components.helpers.ts
,styles.ts
, andindex.ts
.DEFAULT_COLOR_NAME
constant ineditor.ts
.ColorInput
,ColorPopover
, andBrandPalletGroup
to useDEFAULT_COLOR_NAME
.toNormalCase()
instring.ts
to handle fully uppercase strings.This description was created by
for 271a720. It will automatically update as commits are pushed.