-
Notifications
You must be signed in to change notification settings - Fork 854
Added recent used colors in the brand panel #1830
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.
Important
Looks good to me! 👍
Reviewed everything up to 9cf63f2 in 2 minutes and 2 seconds. Click for details.
- Reviewed
107
lines of code in3
files - Skipped
0
files when reviewing. - Skipped posting
10
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/studio/src/lib/editor/engine/theme/index.ts:366
- Draft comment:
Consider adding in-code comments to clarify why addRecentColors() is called here and in similar update flows, preventing accidental duplicate calls. - Reason this comment was not posted:
Confidence changes required:33%
<= threshold50%
None
2. apps/studio/src/lib/editor/engine/theme/index.ts:548
- Draft comment:
The implementation of addRecentColors uses filtering and unshift; consider documenting that the recent colors list is maintained in memory only. - Reason this comment was not posted:
Confidence changes required:33%
<= threshold50%
None
3. apps/studio/src/routes/editor/EditPanel/StylesTab/single/ColorInput/ColorBrandPicker.tsx:127
- Draft comment:
Ensure that using color.lightColor for addRecentColors is consistent with the theme API calls (which use toHex()). - Reason this comment was not posted:
Confidence changes required:33%
<= threshold50%
None
4. apps/studio/src/routes/editor/LayersPanel/BrandTab/index.tsx:81
- Draft comment:
Using array index as key (e.g.,recent-color-${index}
) is acceptable if list order is stable; consider adding unique IDs if order can change. - Reason this comment was not posted:
Confidence changes required:33%
<= threshold50%
None
5. apps/studio/src/lib/editor/engine/theme/index.ts:382
- Draft comment:
Review: The call to reloadWebviews is no longer awaited. Confirm this fire‐and‐forget behavior is intended, as awaiting may be needed for consistency. - 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 comment asks to "confirm" the intention, which violates our rules about not asking for confirmations. 2. The method is called after scanConfig() which suggests it's a UI refresh that doesn't need to block. 3. Without seeing the implementation of reloadWebviews() or understanding the full system architecture, we can't be certain that awaiting is necessary. 4. The code appears to be working as intended since this is a deliberate change. I could be wrong about whether awaiting is necessary - perhaps there are race conditions or UI glitches that could occur without awaiting the reload. While race conditions are possible, we don't have strong evidence that awaiting is required. The change appears deliberate and UI refreshes often don't need to block. Delete this comment. It asks for confirmation rather than pointing out a clear issue, and we don't have strong evidence that awaiting is actually necessary.
6. apps/studio/src/routes/editor/EditPanel/StylesTab/single/ColorInput/ColorBrandPicker.tsx:127
- Draft comment:
Notice: addRecentColors is invoked here on color selection even though update methods also call it. Verify that duplicating this update is intentional and won’t cause unexpected ordering. - Reason this comment was not posted:
Confidence changes required:33%
<= threshold50%
None
7. apps/studio/src/routes/editor/LayersPanel/BrandTab/index.tsx:77
- Draft comment:
Suggestion: Consider conditionally rendering the 'Recent Colors' section (or showing a placeholder) when there are no recent colors to avoid an empty grid display. - Reason this comment was not posted:
Confidence changes required:33%
<= threshold50%
None
8. apps/studio/src/lib/editor/engine/theme/index.ts:366
- Draft comment:
Typo: The comment says 'Base on the class name, find the styles to update'. It would be more clear to say 'Based on the class name, find the styles to update'. - Reason this comment was not posted:
Comment was on unchanged code.
9. apps/studio/src/lib/editor/engine/theme/index.ts:3
- Draft comment:
Potential Typo: The module 'ColorPalletGroup' is imported. If this is intended to reference a color palette group, consider renaming it 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.
10. apps/studio/src/routes/editor/EditPanel/StylesTab/single/ColorInput/ColorBrandPicker.tsx:3
- Draft comment:
Typo alert: The import path uses 'ColorPalletGroup'. Consider verifying if 'Pallet' is intended or if it should be 'Palette', as it's the conventional spelling when referring to a range of colors. - 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_u8BOO0ROQwl3CQhE
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
Hey @SoloDevAbu , this is sick! Yes this can be cached in persistent storage or even in browser-level storage as well. I also DM'ed you on X. |
Hello, I am migrating the desktop app to a new repository. This repository will now focus on the web version. This PR has been migrated to the new repository: onlook-dev#4. Sorry for the inconvenience, rest assure your work here will also be adapted for the web version (if it hasn't already). |
Description
Added recent colors section to show 12 most recently used colors
Related Issues
fixes #1697
Type of Change
Testing
Tested by running locally
Screenshots (if applicable)
Important
Adds a recent colors section to the brand panel, displaying the 12 most recently used colors.
ThemeManager
inindex.ts
to manage recent colors withaddRecentColors()
andrecentColorList
.BrandPopoverPicker
inColorBrandPicker.tsx
to add selected colors to recent colors.BrandTab
inindex.tsx
usingrecentColorList
fromThemeManager
.This description was created by
for 9cf63f2. You can customize this summary. It will automatically update as commits are pushed.