-
Notifications
You must be signed in to change notification settings - Fork 508
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
App Panel, Installed Apps, App Details #1608
base: main
Are you sure you want to change the base?
Conversation
The previous set up only allowed us to append side panels to the LayersPanel index.tsx (if we want to use the blur effect). Using the same backdrop blur on a child when the parent has the blur as well wouldn't work. The new setup lets us append side panels to the specific tab index.tsx and maintain the same background blur. Little confusing - can explain this more if needed.
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 dc27455 in 3 minutes and 35 seconds
More details
- Looked at
1654
lines of code in15
files - Skipped
0
files when reviewing. - Skipped posting
24
drafted comments based on config settings.
1. apps/studio/src/lib/constants/ui.ts:8
- Draft comment:
Good centralization of UI constants. Consider adding additional panel sizes if needed in the future. - Reason this comment was not posted:
Confidence changes required:0%
<= threshold50%
None
2. apps/studio/src/locales/en/translation.json:241
- Draft comment:
Added the 'apps' translation key under brand. Verify consistency with other locales. - Reason this comment was not posted:
Confidence changes required:0%
<= threshold50%
None
3. apps/studio/src/routes/editor/LayersPanel/AppsTab/AppCard.tsx:80
4. apps/studio/src/routes/editor/LayersPanel/AppsTab/DetailPanel/ToolCard.tsx:17
- Draft comment:
Consider extracting the AppIcon component (used as a placeholder) into a shared component to avoid duplication. - Reason this comment was not posted:
Confidence changes required:40%
<= threshold50%
None
5. apps/studio/src/routes/editor/LayersPanel/AppsTab/DetailPanel/index.tsx:176
- Draft comment:
The SVG decoration code is repeated several times. Consider extracting a reusable component or mapping over array of sizes to DRY up the code. - Reason this comment was not posted:
Confidence changes required:50%
<= threshold50%
None
6. apps/studio/src/routes/editor/LayersPanel/AppsTab/InstalledAppCard.tsx:112
- Draft comment:
Looks good; the error indicator dot logic is clearly separated. Ensure integration with app toggle state in future if needed. - Reason this comment was not posted:
Confidence changes required:0%
<= threshold50%
None
7. apps/studio/src/routes/editor/LayersPanel/AppsTab/index.tsx:165
- Draft comment:
Complex state management with hover and selection is handled well. Verify that performance is acceptable when filtering large datasets. - Reason this comment was not posted:
Confidence changes required:0%
<= threshold50%
None
8. apps/studio/src/routes/editor/LayersPanel/BrandTab/ColorPanel/index.tsx:119
- Draft comment:
The processConfigObject function is complex; consider adding inline comments or unit tests for this part to ease maintenance. - Reason this comment was not posted:
Confidence changes required:30%
<= threshold50%
None
9. apps/studio/src/routes/editor/LayersPanel/BrandTab/FontPanel/index.tsx:56
- Draft comment:
Good separation of concerns. Consider adding ARIA labels for accessibility on buttons and inputs. - Reason this comment was not posted:
Confidence changes required:30%
<= threshold50%
None
10. apps/studio/src/routes/editor/LayersPanel/BrandTab/index.tsx:79
- Draft comment:
BrandTab UI looks straightforward. Ensure the repeated use of the same color values is intentional. - Reason this comment was not posted:
Confidence changes required:0%
<= threshold50%
None
11. apps/studio/src/routes/editor/LayersPanel/ImageTab/index.tsx:90
- Draft comment:
Drag and drop logic is well-implemented. Consider resetting pointer events in a finally block to guarantee restoration even if an error occurs during drag end. - Reason this comment was not posted:
Confidence changes required:50%
<= threshold50%
None
12. apps/studio/src/routes/editor/LayersPanel/LayersTab.tsx:60
- Draft comment:
Good use of react-arborist for tree rendering. Ensure dependency arrays for useCallback and useEffect are complete for safe performance. - Reason this comment was not posted:
Confidence changes required:30%
<= threshold50%
None
13. apps/studio/src/routes/editor/LayersPanel/PageTab/index.tsx:35
- Draft comment:
The recursive filterNodes function for search is well crafted. Consider memoizing getDisplayName if the tree grows larger. - Reason this comment was not posted:
Confidence changes required:30%
<= threshold50%
None
14. apps/studio/src/routes/editor/LayersPanel/WindowsTab/index.tsx:28
- Draft comment:
Consider externalizing the duplicated string for window width if used elsewhere. Otherwise, it's clear. - Reason this comment was not posted:
Confidence changes required:20%
<= threshold50%
None
15. apps/studio/src/routes/editor/LayersPanel/index.tsx:25
- Draft comment:
The main LayersPanel composes multiple tabs well. Consider refactoring the mouse leave logic (lines 60-68) for clarity if additional behavior is added later. - Reason this comment was not posted:
Confidence changes required:40%
<= threshold50%
None
16. apps/studio/src/routes/editor/LayersPanel/AppsTab/AppCard.tsx:83
- Draft comment:
Using the CSS filter 'brightness(0) invert(1)' works well for SVG icons. Ensure it also renders non‐SVG images correctly or provide a fallback. - 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%
This is a UI styling concern about how images are rendered. The code comment explicitly states it's for SVGs, suggesting that's the intended use case. The filter technique is a common way to make SVGs white. Even if it doesn't work perfectly for non-SVGs, that would be a UI/styling issue that the author likely considered.
The comment raises a valid technical point - this filter technique might not work well for all image types. There could be edge cases where non-SVG icons look wrong.
However, this is primarily a UI/styling concern, and our rules explicitly state not to comment on UI/styling changes. We should trust that the author tested this with their intended use cases.
Delete this comment as it violates our rule about not commenting on UI/styling changes. We should assume the author has tested the visual appearance appropriately.
17. apps/studio/src/routes/editor/LayersPanel/AppsTab/DetailPanel/index.tsx:76
- Draft comment:
The disable button handler currently only logs a toggle action. Consider providing user feedback (e.g. visual confirmation or a message) when toggling the app state. - 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 comment suggests adding feedback, but the code already has multiple visual indicators: 1) The checkbox icon toggles 2) The "Active" status is shown 3) The entire UI updates through React state. The console.log is likely just for development. Additional feedback would be redundant.
Perhaps there could be a need for an explicit success/error message if the disable action involves an API call that could fail? But there's no evidence of that in the code.
The existing visual feedback through UI state changes is sufficient for a simple toggle action. If backend integration is added later, that would be the appropriate time to add error handling and messaging.
Delete the comment since the code already provides adequate visual feedback through UI state changes and the suggestion would add unnecessary complexity.
18. apps/studio/src/routes/editor/LayersPanel/AppsTab/InstalledAppCard.tsx:68
- Draft comment:
All apps are hard‐coded as enabled by default (isEnabled = true). Consider integrating a dynamic enabled state if the app’s status can change. - 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 isEnabled variable appears to be dead code - it's declared but never used. The onToggle prop exists but handleToggleChange is also never used. This suggests the component is in transition or these features were removed. The comment points out a real issue, but since the code isn't used, fixing it wouldn't impact anything.
The code might be intentionally simplified for now, with plans to add the toggle functionality back later. The hardcoded value might be a temporary solution during refactoring.
However, dead code should be removed rather than fixed. If the toggle functionality is needed later, it can be added back then.
Rather than suggesting to make isEnabled dynamic, the comment should suggest removing the unused isEnabled variable and handleToggleChange function entirely.
19. apps/studio/src/routes/editor/LayersPanel/BrandTab/ColorPanel/index.tsx:118
- Draft comment:
For clarity, consider moving the 'extractVarName' function declaration above 'processConfigObject'. This reduces potential confusion about function availability. - 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.
20. apps/studio/src/routes/editor/LayersPanel/PageTab/index.tsx:40
- Draft comment:
Consider debouncing the search input to reduce frequent re-renders during page filtering. - 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.
21. apps/studio/src/routes/editor/LayersPanel/index.tsx:85
- Draft comment:
The 'isLocked' state logic for tab selection is somewhat unclear. Please add inline comments to clarify its purpose and behavior. - 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.
22. apps/studio/src/routes/editor/LayersPanel/index.tsx:26
- Draft comment:
The PR description mentions adding onboarding documentation for frameworks (Next.js, Remix, Webpack, Babel), but no such documentation changes are evident. Please ensure that the required documentation is included to fully resolve the issue. - 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.
23. apps/studio/src/routes/editor/LayersPanel/BrandTab/ColorPanel/index.tsx:11
- Draft comment:
Typographical note: The component and file are named 'BrandPalletGroup' in the import (line 11) and usage (line 313). Given the context (managing color groups), it might be more appropriate to use 'BrandPaletteGroup' (with an 'e') to align with common terminology for color palettes. If this naming is intentional, please ignore. - 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.
24. apps/studio/src/routes/editor/LayersPanel/index.tsx:13
- Draft comment:
Typo: The component is imported as 'ImagesTab' but the file path is './ImageTab'. Please check if this is intended or if it should be './ImagesTab' to match the usage. - 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_6oKAYxLMTeCPTQ7M
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.
|
||
return ( | ||
<div className="border-b border-border last:border-b-0"> | ||
<div |
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.
Consider adding an aria-expanded
attribute on the expandable card for improved accessibility.
<div | |
<div aria-expanded={isExpanded} |
hasError?: boolean; | ||
} | ||
|
||
// Sample data for app categories |
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.
Consider moving sample app data (APP_CATEGORIES
, FEATURED_APPS
, ALL_APPS
, INSTALLED_APPS
) into a separate module for better maintainability and scalability.
<path | ||
d="M17 8.99994C17 8.48812 16.8047 7.9763 16.4142 7.58579C16.0237 7.19526 15.5118 7 15 7M15 15C18.3137 15 21 12.3137 21 9C21 5.68629 18.3137 3 15 3C11.6863 3 9 5.68629 9 9C9 9.27368 9.01832 9.54308 9.05381 9.80704C9.11218 10.2412 9.14136 10.4583 9.12172 10.5956C9.10125 10.7387 9.0752 10.8157 9.00469 10.9419C8.937 11.063 8.81771 11.1823 8.57913 11.4209L3.46863 16.5314C3.29568 16.7043 3.2092 16.7908 3.14736 16.8917C3.09253 16.9812 3.05213 17.0787 3.02763 17.1808C3 17.2959 3 17.4182 3 17.6627V19.4C3 19.9601 3 20.2401 3.10899 20.454C3.20487 20.6422 3.35785 20.7951 3.54601 20.891C3.75992 21 4.03995 21 4.6 21H7V19H9V17H11L12.5791 15.4209C12.8177 15.1823 12.937 15.063 13.0581 14.9953C13.1843 14.9248 13.2613 14.8987 13.4044 14.8783C13.5417 14.8586 13.7588 14.8878 14.193 14.9462C14.4569 14.9817 14.7263 15 15 15Z" | ||
stroke="white" | ||
stroke-width="1" |
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.
In the SVG <path>
element (around lines 290-297), the attributes stroke-width
, stroke-linecap
, and stroke-linejoin
are written in kebab-case. In React JSX, these should be in camelCase (i.e., strokeWidth
, strokeLinecap
, and strokeLinejoin
) to ensure proper rendering.
Description
Related Issues
Type of Change
Testing
Screenshots (if applicable)
Additional Notes
Important
Adds app management feature with browsing, installation, and detail viewing capabilities in the editor.
AppsTab
component for browsing and managing apps, with tabs for "Browse" and "Installed Apps".AppCard
,InstalledAppCard
, andDetailPanel
components for displaying app information and details.DetailPanel
.PANEL_DIMENSIONS
inui.ts
for consistent panel sizing.translation.json
.LayersPanel
to include the new "Apps" tab.ColorPanel
,FontPanel
,ImageTab
,LayersTab
,PageTab
, andWindowsTab
to usePANEL_DIMENSIONS
.This description was created by
for dc27455. It will automatically update as commits are pushed.