-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Tys toolbar main - In progress #1745
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
padding and radius
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 1c161bc in 3 minutes and 18 seconds
More details
- Looked at
1191
lines of code in11
files - Skipped
0
files when reviewing. - Skipped posting
5
drafted comments based on config settings.
1. packages/ui-v4/src/components/icons/index.tsx:1315
- Draft comment:
The new inline icon definitions (e.g., TextAlignJustified, Width, Height, etc.) are consistent with existing icons. All SVG elements use currentColor correctly, and the use of the cn helper in icons like LeftSide and RightSide maintains consistency. - Reason this comment was not posted:
Confidence changes required:0%
<= threshold50%
None
2. packages/ui-v4/src/components/icons/index.tsx:1765
- Draft comment:
The overall structure using a satisfies clause to enforce icon types is clear and well implemented. No changes required. - Reason this comment was not posted:
Confidence changes required:0%
<= threshold50%
None
3. packages/ui-v4/src/components/icons/index.tsx:1316
- Draft comment:
Icons consistently leverage imported Radix UI icon components and custom SVG definitions. Ensure that any decorative icons used in the UI receive the appropriate accessibility attributes in their usage contexts. - Reason this comment was not posted:
Confidence changes required:10%
<= threshold50%
None
4. apps/web/src/app/project/[id]/_components/editor-bar/text-selected.tsx:22
- Draft comment:
The font name 'Creato Display' in the useState on line 22 seems unusual. Please double-check if 'Creato' is the intended spelling or if it should be corrected. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%
<= threshold50%
The comment is asking the author to double-check the spelling of a font name, which is not allowed according to the rules. The comment should suggest a correction or ask if a specific alternative was intended, but it should not ask the author to double-check.
5. packages/ui-v4/src/components/icons/index.tsx:1655
- Draft comment:
In the BorderEdit SVG, the JSX attributes 'fill-rule' and 'clip-rule' are used. In JSX, these should be in camelCase (i.e., 'fillRule' and 'clipRule') so that React interprets them correctly. - Reason this comment was not posted:
Marked as duplicate.
Workflow ID: wflow_rqUNLuhIgdcWeKmE
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.
fill="currentColor" | ||
/> | ||
<path | ||
fill-rule="evenodd" |
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 BorderEdit
component, the SVG attributes fill-rule
and clip-rule
should be written in camelCase (i.e. fillRule
and clipRule
) to comply with React’s attribute naming conventions.
<DropdownMenuContent align="start" className="w-[280px] mt-2 p-3 rounded-lg space-y-3"> | ||
<div className="space-y-1.5"> | ||
<div className="flex items-center justify-between"> | ||
<span className="text-sm text-muted-white">Width</span> |
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.
Typo alert: The class text-muted-white
on line 62 seems inconsistent with the other similar elements which use text-muted-foreground
. Consider updating to text-muted-foreground
for consistency.
<span className="text-sm text-muted-white">Width</span> | |
<span className="text-sm text-muted-foreground">Width</span> |
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 0fed881 in 3 minutes and 1 seconds
More details
- Looked at
8126
lines of code in100
files - Skipped
0
files when reviewing. - Skipped posting
22
drafted comments based on config settings.
1. package.json:17
- Draft comment:
Issue 123 requires adding onboarding documentation for frameworks (Next.js, Remix, Webpack, Babel), but no documentation files or updates are included in this PR. Please ensure the onboarding docs are added 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.
2. packages/models/src/frame/index.ts:1
- Draft comment:
Frame type definitions are clear and well-typed; no issues found. - Reason this comment was not posted:
Confidence changes required:0%
<= threshold50%
None
3. packages/ui-v4/src/components/resizable.tsx:24
- Draft comment:
The useResizable hook is implemented correctly with proper cleanup. Consider externalizing the transition duration (300 ms) as a constant for easier configuration. - Reason this comment was not posted:
Confidence changes required:33%
<= threshold50%
None
4. packages/utility/src/assert.ts:1
- Draft comment:
The assertNever function is simple and correctly implemented. - Reason this comment was not posted:
Confidence changes required:0%
<= threshold50%
None
5. packages/utility/src/font.ts:187
- Draft comment:
The convertFontString function converts a complex font string as expected. It would be beneficial to add unit tests covering various input edge cases (e.g. multiple fallbacks) to ensure robust behavior. - Reason this comment was not posted:
Confidence changes required:33%
<= threshold50%
None
6. packages/utility/src/index.ts:1
- Draft comment:
The index file re-exports submodules in an organized structure. No changes needed. - Reason this comment was not posted:
Confidence changes required:0%
<= threshold50%
None
7. apps/studio/electron/main/assets/fonts/font.ts:327
- Draft comment:
There's a typographical error in the function declaration of addLocalFont: an extra colon appears after the parameter list (around line 327). This colon should be removed to avoid syntax issues. - 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.
8. apps/studio/package.json:30
- Draft comment:
Typo detected in the scripts section: the key 'pree2e' might be a typo. If this script is intended to run before e2e tests, consider renaming it to 'pre-e2e' or another clear variant 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.
9. apps/studio/src/routes/editor/LayersPanel/BrandTab/ColorPanel/index.tsx:9
- Draft comment:
Typographical suggestion: The component is imported as 'ColorPalletGroup' and used as 'BrandPalletGroup', but usually the term 'palette' (with an 'e') is used when referring to a collection of colors. Consider renaming these to 'ColorPaletteGroup' / 'BrandPaletteGroup' for clarity if this was unintended. - Reason this comment was not posted:
Comment was on unchanged code.
10. apps/studio/src/routes/editor/LayersPanel/WindowsTab/FrameDimensions.tsx:381
- Draft comment:
Typo: The second Orientation button is comparing using the string 'Landscape' instead of using Orientation.Landscape like elsewhere. Consider using Orientation.Landscape for 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.
11. apps/studio/src/routes/editor/LayersPanel/WindowsTab/FrameDimensions.tsx:394
- Draft comment:
Typo: The CSS class 'text-foreground-quadranary' appears to be misspelled. If the intended name is 'text-foreground-quaternary', please update it. - 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.
12. apps/studio/src/routes/editor/LayersPanel/WindowsTab/FrameDimensions.tsx:429
- Draft comment:
Typo: The CSS class 'text-foreground-quadranary' is likely misspelled. If 'text-foreground-quaternary' is intended, please update it for 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.
13. apps/web/src/app/layout.tsx:38
- Draft comment:
Extraneous space detected in the closing HTML tag ''. Please remove the extra space to ensure consistency with typical HTML formatting. - Reason this comment was not posted:
Comment was on unchanged code.
14. apps/web/src/app/project/[id]/_components/canvas/overlay/pan.tsx:43
- Draft comment:
Minor: Consider removing the trailing space in the string 'visible ' (line 43) to ensure consistency in class names. This is a trivial typographical error. - 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%
While technically correct about the trailing space, this is an extremely minor issue. CSS class names are typically trimmed by browsers and the cn() utility likely handles this. The space won't cause any functional issues. This feels like unnecessary nitpicking that doesn't improve code quality in any meaningful way.
The trailing space could theoretically cause confusion for other developers or lead to inconsistency in the codebase. Maybe it's worth maintaining strict cleanliness in class names?
The potential benefits of fixing this are vastly outweighed by the cognitive overhead of reviewing and addressing the comment. This is the kind of micro-optimization that distracts from more important code review concerns.
Delete this comment as it points out an extremely minor formatting issue that has no functional impact and isn't worth the developer's time to address.
15. apps/web/src/app/project/[id]/_components/right-panel/chat/panel-dropdown.tsx:75
- Draft comment:
Typographical error: The label 'Auto - apply results' contains extra spaces around the dash. Consider changing it to 'Auto-apply results' 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%
While this is technically correct about inconsistent hyphenation, it falls into the category of pure UI/styling changes. The rules explicitly state not to comment on UI or styling changes, assuming the author made these choices deliberately. The hyphenation difference is a minor visual detail that doesn't affect functionality.
The inconsistent hyphenation could potentially affect searchability or accessibility of the UI element. It might also indicate inconsistency with other parts of the codebase's style.
While consistency is good, this is still fundamentally a UI styling choice. The rules are clear about not commenting on UI changes, and this doesn't affect the logic or functionality of the code.
Delete this comment as it violates the rule about not commenting on UI and styling changes.
16. apps/web/src/app/project/[id]/_components/right-panel/index.tsx:122
- Draft comment:
Typographical error: Remove the extra space between '<' and 'Icons.ChevronDown' to ensure consistent JSX syntax. - 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%
While technically correct that there's an extra space, this is an extremely minor formatting issue. It doesn't affect functionality at all. The file shows inconsistent spacing throughout, suggesting this isn't enforced by the team's style guide. This kind of nitpicky formatting comment creates noise without adding value.
The comment is technically accurate - there is an extra space. And consistency in code formatting can be important for readability.
However, this is too minor of an issue to warrant a PR comment. This kind of formatting should be handled by automated tools like Prettier rather than manual review comments.
Delete this comment. Minor formatting issues like extra spaces should be handled by automated formatting tools, not PR comments.
17. apps/web/src/app/project/[id]/_components/right-panel/index.tsx:125
- Draft comment:
Typographical error: Remove the extra space between '<' and 'TabsTrigger' to standardize the JSX tag formatting. - 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%
While the comment is technically correct about inconsistent formatting, this is a very minor stylistic issue that would typically be handled by code formatters like Prettier. The issue appears multiple times, suggesting it might be an intentional style choice or something that should be handled systematically rather than with individual comments.
The formatting inconsistency could potentially indicate a deeper issue with code formatting tooling or team standards. Multiple instances suggest a systematic rather than one-off issue.
However, this is exactly the kind of minor formatting issue that should be handled by automated tools rather than manual code review comments. It doesn't affect functionality and isn't worth the reviewer's or author's time.
Delete the comment. This type of minor formatting issue should be handled by automated formatting tools rather than manual review comments.
18. apps/web/src/app/project/[id]/_components/right-panel/index.tsx:132
- Draft comment:
Typographical error: Remove the unnecessary space before 'TabsTrigger' to maintain consistency in the JSX markup. - 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%
While the comment is technically correct about inconsistent spacing, this is a very minor stylistic issue. This kind of formatting is typically handled automatically by code formatters like Prettier. Making developers manually fix spacing is not a good use of review comments. The change doesn't impact functionality or readability significantly.
The inconsistent spacing could indicate that the code formatter is not properly configured or being used. This could lead to more formatting inconsistencies over time.
Even if there are formatter issues, those should be handled at a project configuration level, not through individual PR comments about spaces.
This comment should be deleted as it focuses on a minor stylistic issue that would be better handled by automated tooling rather than manual code review comments.
19. apps/web/src/app/project/[id]/_components/right-panel/index.tsx:148
- Draft comment:
Typographical error: Remove the leading space before 'TabsContent' to comply with typical JSX element formatting. - 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 points out a formatting issue, but the leading spaces appear to be an intentional style choice used consistently throughout the file. Making this change would actually make the code less consistent. Additionally, this is a minor formatting issue that could be handled by a code formatter if the team wants to enforce a particular style.
Maybe there's a team-wide style guide that prefers no leading spaces before JSX elements that I'm not aware of. The consistent style in this file could be violating team conventions.
Even if there is a team style guide, this kind of minor formatting issue should be handled by automated tooling like Prettier or ESLint, not through code review comments.
Delete this comment. It's a minor formatting issue that should be handled by automated tooling, and the suggested change would actually make the code less consistent with the file's current style.
20. apps/web/src/app/project/[id]/_components/right-panel/index.tsx:151
- Draft comment:
Typographical error: Remove the unnecessary leading space before 'TabsContent' for consistent JSX tag formatting. - 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 identifies a real inconsistency in the code formatting. However, this is a very minor stylistic issue that would typically be handled by a code formatter like Prettier. It doesn't affect functionality and isn't a significant code quality issue that requires manual intervention.
The comment is technically correct about the inconsistency, but am I being too dismissive of formatting consistency? Even small inconsistencies can make code harder to maintain over time.
While consistency is important, this level of formatting detail should be handled by automated tools rather than manual code review comments. It creates noise in the review process.
This comment should be removed as it addresses a minor formatting issue that would be better handled by automated formatting tools rather than manual code review.
21. packages/ui-v4/src/components/icons/index.tsx:1041
- Draft comment:
Typo: 'Potrait' should be corrected to 'Portrait' for consistency and clarity. - Reason this comment was not posted:
Comment looked like it was already resolved.
22. packages/ui/src/components/icons/index.tsx:1041
- Draft comment:
Typo fixed: Changed the icon name from 'Potrait' to 'Portrait' to correct the spelling. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%
<= threshold50%
This comment is purely informative, as it only states that a typo was fixed. It doesn't provide any actionable feedback or suggestions for improvement.
Workflow ID: wflow_lcjGkVArhtQFLB0J
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 8346467 in 1 minute and 29 seconds
More details
- Looked at
97
lines of code in2
files - Skipped
0
files when reviewing. - Skipped posting
6
drafted comments based on config settings.
1. apps/web/src/app/project/[id]/_components/editor-bar/staging-toggle.tsx:15
- Draft comment:
Consider refactoring the repeated button markup (div, text, image) using a mapping over element types to improve maintainability. - Reason this comment was not posted:
Confidence changes required:50%
<= threshold50%
None
2. apps/web/src/app/project/[id]/page.tsx:38
- Draft comment:
PR description mentions better onboarding docs, but no documentation or related content is added in the diff. - 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/web/src/app/project/[id]/_components/editor-bar/staging-toggle.tsx:15
- Draft comment:
Add type='button' and aria-label attributes to each to improve accessibility and prevent accidental form submissions if these buttons ever reside in a form. - Reason this comment was not posted:
Confidence changes required:33%
<= threshold50%
None
4. apps/web/src/app/project/[id]/_components/editor-bar/staging-toggle.tsx:17
- Draft comment:
The conditional className logic is duplicated across the three buttons. Consider refactoring this (e.g. using a helper or mapping over element types) to improve maintainability. - Reason this comment was not posted:
Confidence changes required:33%
<= threshold50%
None
5. apps/web/src/app/project/[id]/page.tsx:46
- Draft comment:
Verify the stacking order: the StagingToggle container uses a z-index of 'z-1', which is below the EditorBar's 'z-10'. Confirm that this layering is intentional, especially since Tailwind’s default z-index scale may not include 'z-1'. - Reason this comment was not posted:
Confidence changes required:33%
<= threshold50%
None
6. apps/web/src/app/project/[id]/page.tsx:50
- Draft comment:
The PR description mentions onboarding docs improvements, but no documentation changes are present in this diff. Please ensure that the onboarding documentation updates are added or update the description accordingly. - Reason this comment was not posted:
Confidence changes required:33%
<= threshold50%
None
Workflow ID: wflow_JBBN8Df5ROZbGt5t
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
* Text selected base - Size input * Updated button styling padding and radius * Weights dropdown & new styling * Text alignment dropdown added * svg added * Updated icons and added Text Color base * Selection toggle * Div selected dropdown base * Updated dropdown styling * Updated styling and icon placeholders * New icons added * Added icons * Added input component * Fix height issue
* Text selected base - Size input * Updated button styling padding and radius * Weights dropdown & new styling * Text alignment dropdown added * svg added * Updated icons and added Text Color base * Selection toggle * Div selected dropdown base * Updated dropdown styling * Updated styling and icon placeholders * New icons added * Added icons * Added input component * Fix height issue
Description
Related Issues
Type of Change
Testing
Screenshots (if applicable)
Additional Notes
Important
Introduces a new editor toolbar with various UI components and updates dependencies and icons.
DivSelected
,TextSelected
,EditorBar
,StagingToggle
,StateDropdown
, andInputDropdown
components inapps/web/src/app/project/[id]/_components/editor-bar/
.EditorBar
displays different UI based onselectedElement
prop (div
,text
,image
).StagingToggle
allows toggling betweendiv
,text
, andimage
elements.TextAlignJustified
,TextColorSymbol
,Width
,Height
,Padding
,Margin
,CornerRadius
,Layout
,StateCursor
,BorderEdit
,BackgroundImage
,LeftSide
,RightSide
,TopSide
,BottomSide
inpackages/ui-v4/src/components/icons/index.tsx
.tailwind-merge
from^3.1.0
to^3.2.0
inapps/web/package.json
andbun.lock
.TopBar
intop-bar/index.tsx
to adjust styling and remove unused code.This description was created by
for 8346467. It will automatically update as commits are pushed.