-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Tys toolbar v3 - halfway mark #1764
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.
❌ Changes requested. Reviewed everything up to 6498e9d in 3 minutes and 40 seconds
More details
- Looked at
1303
lines of code in16
files - Skipped
0
files when reviewing. - Skipped posting
8
drafted comments based on config settings.
1. packages/ui-v4/src/components/icons/index.tsx:122
- Draft comment:
Many icon components use the pattern cn('', className). Since className is already passed in, consider whether an empty string is needed or if directly passing className (or using cn(className)) would be clearer. - 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.
2. packages/ui-v4/src/components/icons/index.tsx:122
- Draft comment:
Consider adding aria-hidden="true" to icon SVGs if they are purely decorative to improve accessibility. - 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. packages/ui-v4/src/components/icons/index.tsx:137
- Draft comment:
Consider adding an aria-hidden attribute (e.g. aria-hidden="true") to the root elements to enhance accessibility for decorative icons. - 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.
4. packages/ui-v4/src/components/icons/index.tsx:1763
- Draft comment:
CornerTopRight is implemented inline while CornerTopLeft uses an imported icon. For consistency, ensure all corner icons follow the same pattern or design source. - 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%
While the comment identifies a real inconsistency, it's not clear if this is actually a problem that needs fixing. The icons appear to have the same functionality regardless of implementation. The change seems intentional since multiple corner icons were moved to inline implementations. Without more context about why consistency is important here, this feels like a nitpick rather than a substantive issue.
The comment does identify a real pattern inconsistency. Perhaps there are performance or maintainability benefits to having a consistent implementation approach.
While consistency can be good, the change appears intentional and both approaches work fine. The comment doesn't explain why consistency is important in this case.
This comment should be deleted as it points out an inconsistency without demonstrating why it's problematic. The change appears intentional and both implementation approaches are valid.
5. packages/ui-v4/src/components/icons/index.tsx:1820
- Draft comment:
This file is quite long with many inline icon definitions. Consider splitting the icons into separate modules to improve maintainability and readability. - Reason this comment was not posted:
Comment was on unchanged code.
6. packages/ui-v4/src/components/icons/index.tsx:1620
- Draft comment:
If these icons are rendered frequently in dynamic contexts, consider wrapping them with React.memo to prevent unnecessary re-renders. - Reason this comment was not posted:
Confidence changes required:50%
<= threshold50%
None
7. apps/web/client/src/app/project/[id]/_components/editor-bar/dropdowns/width.tsx:27
- Draft comment:
Typographical error: The class name 'text-muted-white' on line 27 appears inconsistent with similar classes (e.g., 'text-muted-foreground' used in other elements). Consider updating it to 'text-muted-foreground' to maintain consistency unless 'text-muted-white' is intentional. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 0% vs. threshold = 50%
This is a new file being added, and the comment is about UI styling consistency. The rules explicitly state "Do NOT comment on any pure frontend UI or styling changes, only logic. Assume that if the author changed the UI, they did it correctly." Even though the inconsistency observation seems valid, it falls under the category of UI styling that we should not comment on.
The inconsistency could be intentional - perhaps the "Width" label specifically needs to be styled differently from the "Min" and "Max" labels for UX reasons.
Even if the styling difference is unintentional, the rules clearly state we should not comment on UI styling changes.
Delete this comment as it violates the rule about not commenting on UI and styling changes.
8. apps/web/client/src/app/project/[id]/_components/left-panel/index.tsx:12
- Draft comment:
In the commented import at line 12, consider correcting 'import ImagesTab from './ImageTab';' to 'import ImagesTab from './ImagesTab';' for consistency with the later usage of . - 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_RpzFFznXp0Vj1W1q
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.
<EditorBar selectedElement={selectedElement} /> | ||
</div> | ||
|
||
<div className="absolute top-20 w-full"> | ||
<div className="absolute top-20 w-screen h-screen flex items-center justify-center z-30"> |
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.
fyi this is what is causing the weird scroll behavior. I know it's for staging.
|
||
<div className="absolute top-10 left-0 animate-layer-panel-in h-[calc(100%-2.60rem)] z-1"> | ||
{/* Height full minus top bar and editor bar (80px) */} | ||
<div className="absolute top-20 left-0 animate-layer-panel-in h-[calc(100%-80px)] z-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.
@tycreated fyi:
Instead of adding the padding on the inner element, we should have these pushed down to accommodate the new element and sized accordingly.
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 75aa3c9 in 2 minutes and 9 seconds
More details
- Looked at
46
lines of code in2
files - Skipped
0
files when reviewing. - Skipped posting
7
drafted comments based on config settings.
1. apps/web/client/src/app/project/[id]/_components/left-panel/index.tsx:112
- Draft comment:
Removed 'pt-12' (top padding) from the container. Confirm this change is intentional for the layout. - Reason this comment was not posted:
Confidence changes required:33%
<= threshold50%
None
2. apps/web/client/src/app/project/[id]/page.tsx:33
- Draft comment:
The import and usage of StagingToggle have been commented out. Remove dead code if it’s no longer needed. - Reason this comment was not posted:
Confidence changes required:33%
<= threshold50%
None
3. apps/web/client/src/app/project/[id]/page.tsx:38
- Draft comment:
Updated panel positioning: top offset changed from 10px to 20px and height from 'calc(100%-2.60rem)' to 'calc(100%-80px)'. Verify these values meet design requirements. - Reason this comment was not posted:
Confidence changes required:33%
<= threshold50%
None
4. apps/web/client/src/app/project/[id]/_components/left-panel/index.tsx:112
- Draft comment:
Removed 'pt-12' may affect vertical spacing. Confirm this change is intentional. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 0% vs. threshold = 50%
The rules explicitly state "Do NOT comment on any pure frontend UI or styling changes, only logic. Assume that if the author changed the UI, they did it correctly." This is clearly a UI styling change, removing padding. Additionally, the comment asks for confirmation ("Confirm this change is intentional") which is another rule violation.
Perhaps the padding change could affect some critical functionality or accessibility that I'm not considering? Maybe there's some layout-dependent logic I'm missing?
Even if there were layout-dependent logic, the rules clearly state to trust the author on UI changes. Additionally, asking for confirmation is explicitly discouraged.
This comment should be deleted as it violates two rules: commenting on UI changes and asking for confirmation of intent.
5. apps/web/client/src/app/project/[id]/page.tsx:10
- Draft comment:
Removed import for StagingToggle. Verify this component is no longer needed. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%
<= threshold50%
The comment is asking the PR author to verify that a removed import is no longer needed. This falls under asking the author to confirm their intention, which is against the rules.
6. apps/web/client/src/app/project/[id]/page.tsx:38
- Draft comment:
Updated left panel positioning uses 'top-20' and height 'calc(100%-80px)'. Ensure these values correctly match the intended TopBar/EditorBar heights. Consider using spaces in the calc (e.g., 'calc(100% - 80px)'). - 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 has two parts: 1) Adding spaces in calc() which is a minor styling preference 2) Verifying heights which violates the rule about asking authors to verify/confirm things. The code already has a clear comment explaining the 80px value. This is a UI layout change and we should trust the author's implementation.
The calc() spacing suggestion could improve readability. Maybe there's an actual functional difference between calc() with and without spaces that I'm not considering.
Spaces in calc() are purely stylistic and both versions work identically in CSS. The verification request violates our rules.
Delete this comment as it combines a minor style preference with a request for verification, which violates our rules about not asking authors to verify things.
7. apps/web/client/src/app/project/[id]/page.tsx:42
- Draft comment:
Similarly, right panel positioning is updated to 'top-20' and 'calc(100%-80px)'. Validate consistency across components and consider removing commented-out code if no longer needed. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =50%
<= threshold50%
The comment suggests validating consistency across components and removing commented-out code if it's no longer needed. The first part of the comment is a suggestion to ensure consistency, which is not allowed. However, the second part about removing commented-out code is a valid suggestion if the code is indeed unnecessary.
Workflow ID: wflow_oofvJEvtgbBl1Bke
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
* Padding base * Margin dropdown & corner base * Corner dropdown and icons * New dropdowns * Border dropdown * Updated input with unit * Range slider base * Range input component * Update input-range.tsx * Input range update * Removed chevrons and updated padding * Removed text chevrons and updated padding * Removed state chevron * Dropdowns organized * Updated image bar * Update img-selected.tsx * Fix responsive sizing
* Padding base * Margin dropdown & corner base * Corner dropdown and icons * New dropdowns * Border dropdown * Updated input with unit * Range slider base * Range input component * Update input-range.tsx * Input range update * Removed chevrons and updated padding * Removed text chevrons and updated padding * Removed state chevron * Dropdowns organized * Updated image bar * Update img-selected.tsx * Fix responsive sizing
Description
Related Issues
Type of Change
Testing
Screenshots (if applicable)
Additional Notes
Important
Enhance toolbar with new dropdown components for styling and update icon set and package versions.
ImgSelected
component inimg-selected.tsx
for image selection toolbar.Height
,Width
,Padding
,Margin
,Radius
in respective files for styling options.DivSelected
indiv-selected.tsx
to include new dropdowns for layout options.CornerTopRight
,CornerBottomRight
,CornerBottomLeft
icons inindex.tsx
.CornerTopRight
,CornerBottomRight
,CornerBottomLeft
icons inindex.tsx
.EditorBar
inindex.tsx
to includeImgSelected
component.bun.lock
to reflect new package versions for@ai-sdk
,dotenv
,eslint
,sharp
,supabase
, and others.This description was created by
for 75aa3c9. It will automatically update as commits are pushed.