Tys toolbar v4 #1786
Merged
Tys toolbar v4 #1786
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
![ellipsis-dev[bot]](https://avatars.githubusercontent.com/in/64358?s=60&v=4){kind=small}
There was a problem hiding this comment.
❌ Changes requested. Reviewed everything up to 1e4c4de in 3 minutes and 14 seconds
More details
- Looked at
1912lines of code in22files - Skipped
0files when reviewing. - Skipped posting
16drafted comments based on config settings.
1. packages/ui-v4/src/components/icons/index.tsx:1634
- Draft comment:
BorderEdit icon now uses width=16, height=12 with a different viewBox and rect paths. Confirm that these new dimensions and style are intentional and consistent with the rest of the icon set. - 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 their intention regarding the dimensions and style of an icon. This violates the rule against asking for confirmation of intention. However, it does point out a specific change in dimensions and style, which could be useful if it suggests a potential inconsistency. The comment could be rephrased to suggest checking for consistency without asking for confirmation.
2. apps/web/client/src/app/project/[id]/page.tsx:30
- Draft comment:
StagingToggle was previously commented out. Ensure that its integration (layering/z-index) is fully reviewed and intentional. - Reason this comment was not posted:
Confidence changes required:50%<= threshold50%
None
3. apps/web/client/src/app/project/[id]/_components/editor-bar/panels/layout-position.tsx:60
- Draft comment:
Consider extracting repeated magic numbers (e.g. w-[225px]) into constants or theme values to maintain consistency. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%
None
4. PR Description:1
- Draft comment:
The PR description mentions adding better onboarding docs (Next.js, Remix, Webpack, Babel) but the changes only reflect UI toolbar/panels enhancements. Please update the description or add the missing documentation changes. - 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/web/client/src/app/project/[id]/_components/editor-bar/div-selected.tsx:20
- Draft comment:
Refactor: The old Flex dropdown menu was replaced by the component. Please verify that this change preserves all intended functionality. - Reason this comment was not posted:
Confidence changes required:50%<= threshold50%
None
6. apps/web/client/src/app/project/[id]/_components/editor-bar/dropdowns/border.tsx:4
- Draft comment:
Consider using a union type (e.g. 'all' | 'individual') for the activeTab state for improved type safety. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%
None
7. apps/web/client/src/app/project/[id]/_components/editor-bar/dropdowns/img-background.tsx:15
- Draft comment:
Inline styling for the background image pattern could be extracted into a constant for better readability. - Reason this comment was not posted:
Confidence changes required:50%<= threshold50%
None
8. apps/web/client/src/app/project/[id]/_components/editor-bar/inputs/input-color.tsx:12
- Draft comment:
For improved accessibility, consider adding aria-labels or placeholders to these input fields. - Reason this comment was not posted:
Confidence changes required:50%<= threshold50%
None
9. apps/web/client/src/app/project/[id]/_components/editor-bar/inputs/input-dropdown.tsx:29
- Draft comment:
The text input width was reduced (from 40px to 32px); please confirm this change still offers sufficient space for user input. - Reason this comment was not posted:
Confidence changes required:50%<= threshold50%
None
10. apps/web/client/src/app/project/[id]/_components/editor-bar/inputs/input-icon.tsx:38
- Draft comment:
Now that unit selection is enabled via a dropdown, consider providing a default onUnitChange handler to avoid unexpected behavior when none is supplied. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%
None
11. apps/web/client/src/app/project/[id]/_components/editor-bar/inputs/input-radio.tsx:8
- Draft comment:
The radio input implementation is solid. Good job handling both icon and text options. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%
None
12. apps/web/client/src/app/project/[id]/page.tsx:35
- Draft comment:
New Panels are positioned absolutely. Please verify that this layout does not conflict with the existing LeftPanel/RightPanel on different screen sizes. - Reason this comment was not posted:
Confidence changes required:50%<= threshold50%
None
13. packages/ui-v4/src/components/icons/index.tsx:1635
- Draft comment:
The BorderEdit icon dimensions have changed from 24x24 to 16x12. Confirm that this scaling aligns with design guidelines and maintains visual consistency. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%
The comment is asking the PR author to confirm that the change in icon dimensions aligns with design guidelines and maintains visual consistency. This falls under asking the author to confirm their intention or ensure behavior, which is against the rules.
14. packages/ui-v4/src/components/icons/index.tsx:1815
- Draft comment:
A new PaintBucket icon has been added. Please ensure its clarity and proper scaling within the toolbar UI. - Reason this comment was not posted:
Confidence changes required:50%<= threshold50%
None
15. apps/web/client/src/app/project/[id]/page.tsx:10
- Draft comment:
The issue description mentions onboarding documentation updates, but the diff primarily includes toolbar refactoring. Please ensure that the documentation updates for Next.js, Remix, Webpack, and Babel are included or linked elsewhere. - 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.
16. apps/web/client/src/app/project/[id]/_components/editor-bar/panels/index.tsx:31
- Draft comment:
Please add a newline at the end of the file to adhere to common file formatting standards. - 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 having a newline at the end of files is a common convention, this kind of formatting issue is typically handled by code formatters like Prettier or EditorConfig. It's a very minor issue that doesn't affect functionality. According to the rules, we shouldn't make comments about obvious or unimportant things that would be caught by build tools.
Maybe this is actually an important POSIX compliance issue that could cause problems in some environments? Some tools do require trailing newlines.
While POSIX compliance is important, this is still the kind of issue that should be handled by automated tooling rather than manual review comments. If it's important for the project, it should be enforced by the build system.
This comment should be deleted as it's a minor formatting issue that should be handled by automated tools rather than manual review.
Workflow ID: wflow_xHkszwRR1IBNdU72
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.
| <div className="rounded-lg bg-background backdrop-blur p-4"> | ||
| <div className="flex items-center justify-between mb-0 cursor-pointer" onClick={() => setLayoutOpen(!layoutOpen)}> | ||
| <span className="text-sm font-medium text-white w-24">Layout</span> | ||
| <Icons.ChevronUp className={cn("h-4 w-4 text-muted-foreground transition-transform", !layoutOpen && "rotate-180")} /> |
There was a problem hiding this comment.
Consider using a semantic <button> rather than a clickable <div> for collapsible sections to improve accessibility.
| {/* Font Family */} | ||
| <div className="flex items-center justify-between"> | ||
| <span className="text-sm text-muted-foreground w-24">Font</span> | ||
| <div className="flex-1"> |
There was a problem hiding this comment.
The Font Family field currently renders a button without an associated interaction. Please ensure this is either made interactive or clearly marked as a placeholder.
There was a problem hiding this comment.
❌ Changes requested. Reviewed everything up to 1e4c4de in 3 minutes and 58 seconds
More details
- Looked at
1912lines of code in22files - Skipped
0files when reviewing. - Skipped posting
9drafted comments based on config settings.
1. apps/web/client/src/app/project/[id]/page.tsx:28
- Draft comment:
The PR description mentions adding onboarding docs for frameworks, but no documentation updates were found in this diff. Please confirm if the docs changes are in a separate commit. - 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. apps/web/client/src/app/project/[id]/_components/editor-bar/inputs/input-icon.tsx:16
- Draft comment:
Consider moving the UNITS array outside the component to avoid re-creation on every render. - Reason this comment was not posted:
Comment looked like it was already resolved.
3. packages/ui-v4/src/components/icons/index.tsx:1634
- Draft comment:
The PaintBucket icon uses complex inline styles; consider extracting these styles to a CSS class for better maintainability and possible reuse. - Reason this comment was not posted:
Comment looked like it was already resolved.
4. apps/web/client/src/app/project/[id]/_components/editor-bar/img-background.tsx:8
- Draft comment:
Inline styles in the ImageBackground component could be extracted to CSS for improved consistency and easier theming. - 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/web/client/src/app/project/[id]/page.tsx:32
- Draft comment:
The Panels component is now added with a high z-index. Please verify that the layering (z-index) and positioning work correctly with existing panels. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%
None
6. apps/web/client/src/app/project/[id]/page.tsx:32
- Draft comment:
The PR description mentions onboarding docs improvements, but the changes primarily update toolbar/UI components. Confirm that the issue scope is correctly described. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%
This comment asks the PR author to confirm the issue scope described in the PR description, which violates the rule against asking the author to update or confirm the PR description. The comment does not provide a specific code suggestion or ask for a test to be written, so it should be removed.
7. packages/ui-v4/src/components/icons/index.tsx:1634
- Draft comment:
BorderEdit icon dimensions and implementation have changed (now using a 16x12 viewBox with rect elements). Please verify that this scaling and style change is aligned with the design guidelines. - Reason this comment was not posted:
Confidence changes required:50%<= threshold50%
None
8. apps/web/client/src/app/project/[id]/_components/editor-bar/inputs/input-dropdown.tsx:28
- Draft comment:
InputDropdown widths and paddings were reduced (e.g., from 80px to a min-width of 72px and input width from 40px to 32px). Please double-check that these adjustments meet the intended design specifications. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%
None
9. apps/web/client/src/app/project/[id]/_components/editor-bar/panels/index.tsx:31
- Draft comment:
There is no newline at the end of the file. Please add a newline at the end to follow standard file formatting practices. - 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 missing newlines at end of file can cause issues with some tools, this is something that should be caught by linting or formatting tools. It's a very minor issue that doesn't affect functionality. According to the rules, we shouldn't comment on pure formatting issues that would be caught by the build.
Missing newlines at end of file can cause issues with git and some Unix tools, so this could be a legitimate concern.
While true, this is exactly the kind of minor formatting issue that should be handled automatically by the project's linting/formatting tools rather than manual review comments.
The comment should be deleted as it addresses a minor formatting issue that should be handled by automated tools rather than code review.
Workflow ID: wflow_xHkszwRR1IBNdU72
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.
| const [direction, setDirection] = useState<"vertical" | "horizontal">("horizontal"); | ||
| const [verticalAlign, setVerticalAlign] = useState<"top" | "center" | "bottom" | "space-between">("center"); | ||
| const [horizontalAlign, setHorizontalAlign] = useState<"left" | "center" | "right" | "space-between">("center"); | ||
| const [gap, setGap] = useState("12"); |
There was a problem hiding this comment.
Unused gap state: const [gap, setGap] = useState("12") is declared but not applied. The InputIcon component for Gap is hardcoded with value 18. Consider using the gap state or removing it.
Suggested change
| const [gap, setGap] = useState("12"); |
| import { DropdownMenu, DropdownMenuContent, DropdownMenuItem, DropdownMenuTrigger } from "@onlook/ui-v4/dropdown-menu"; | ||
|
|
||
| const UNITS = ["PX", "%", "REM", "VW", "VH"] as const; | ||
| type Unit = typeof UNITS[number]; | ||
|
|
||
| interface InputIconProps { |
There was a problem hiding this comment.
The InputIcon component no longer accepts an icon prop, but it is still being used (e.g. in LayoutPosition and TextSelected) with an icon argument. This likely is a breaking change – update the component or its usages accordingly.
| fill="#929292" | ||
| /> | ||
| <path | ||
| fill-rule="evenodd" |
There was a problem hiding this comment.
Typo: The attributes fill-rule and clip-rule in the PaintBucket icon should be updated to fillRule and clipRule for proper JSX attribute naming.
Suggested change
| fill-rule="evenodd" | |
| fillRule="evenodd" |
There was a problem hiding this comment.
@tycreated fyi since this is valid jsx/tsx which is different from html, we'll use camelCase vs kebob-case. Seems like the rest look good through will fix.
There was a problem hiding this comment.
❌ Changes requested. Incremental review on 6f502a8 in 2 minutes and 25 seconds
More details
- Looked at
95lines of code in5files - Skipped
0files when reviewing. - Skipped posting
8drafted comments based on config settings.
1. apps/web/client/src/app/project/[id]/_components/editor-bar/img-selected.tsx:70
- Draft comment:
For consistency with other controls, consider adding a divider before (like other elements) if intended. - Reason this comment was not posted:
Confidence changes required:50%<= threshold50%
None
2. apps/web/client/src/app/project/[id]/_components/editor-bar/panels/panel-bar/button.tsx:21
- Draft comment:
Consider extracting or commenting complex class strings for maintainability. - Reason this comment was not posted:
Confidence changes required:30%<= threshold50%
None
3. apps/web/client/src/app/project/[id]/_components/editor-bar/text-selected.tsx:221
- Draft comment:
Ensure that placing here (without a divider) aligns with the intended design consistency across the toolbar. - Reason this comment was not posted:
Confidence changes required:50%<= threshold50%
None
4. apps/web/client/src/app/project/[id]/_components/editor-bar/img-selected.tsx:71
- Draft comment:
Ensure the new placement and styling aligns with the toolbar design. - Reason this comment was not posted:
Confidence changes required:30%<= threshold50%
None
5. apps/web/client/src/app/project/[id]/_components/editor-bar/panels/panel-bar/button.tsx:21
- Draft comment:
Consider refactoring the long Tailwind class string for maintainability—extract common styles if possible. - Reason this comment was not posted:
Confidence changes required:30%<= threshold50%
None
6. apps/web/client/src/app/project/[id]/_components/editor-bar/panels/typography.tsx:12
- Draft comment:
Remove the unused import of ViewButtons if it's not being utilized in this component. - 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 real issue - there's an unused import. However, this kind of issue would typically be caught by ESLint or TypeScript. Most IDEs would also highlight this. It's a very basic code quality issue that doesn't require human review.
The comment is technically correct - the import is unused. But am I being too quick to dismiss it just because it could be caught by tools?
While correct, these kinds of issues are better handled by automated tools. The PR review comments should focus on more substantial issues that require human judgment.
Delete the comment since unused imports are better caught by automated tools like ESLint or TypeScript, not human reviewers.
7. apps/web/client/src/app/project/[id]/_components/editor-bar/text-selected.tsx:221
- Draft comment:
Verify that the integration of maintains consistent spacing and alignment with the rest of the toolbar. - Reason this comment was not posted:
Confidence changes required:30%<= threshold50%
None
8. apps/web/client/src/app/project/[id]/_components/editor-bar/text-selected.tsx:22
- Draft comment:
Typo alert: The string 'Creato Display' on line 22 appears to have a typographical error. Please verify if 'Creato' should be corrected (e.g., 'Creative Display' or another intended name). - 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_7ZNYtWmdBUZmlWMQ
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.
| import { InputColor } from "../inputs/input-color"; | ||
| import { Button } from "@onlook/ui-v4/button"; | ||
| import { DropdownMenu, DropdownMenuTrigger, DropdownMenuContent, DropdownMenuItem } from "@onlook/ui-v4/dropdown-menu"; | ||
| import { ViewButtons } from "./panel-bar/bar"; |
There was a problem hiding this comment.
Remove the unused import for ViewButtons if it’s not used in this component.
Suggested change
| import { ViewButtons } from "./panel-bar/bar"; |
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on 91b1c60 in 1 minute and 10 seconds
More details
- Looked at
15lines of code in1files - Skipped
0files when reviewing. - Skipped posting
3drafted comments based on config settings.
1. apps/web/client/src/app/project/[id]/_components/editor-bar/panels/index.tsx:26
- Draft comment:
Confirm the swap of 'hidden' from LayoutPosition to Typography is intentional; the UI behavior may change. - Reason this comment was not posted:
Comment looked like it was already resolved.
2. apps/web/client/src/app/project/[id]/_components/editor-bar/panels/index.tsx:26
- Draft comment:
Verify that these UI changes align with the intended onboarding/documentation improvements. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%
This comment is asking the PR author to verify alignment with intended improvements, which is against the rules as it asks for confirmation of intention.
3. apps/web/client/src/app/project/[id]/_components/editor-bar/panels/index.tsx:26
- Draft comment:
Swapped hidden classes: LayoutPosition is now visible and Typography is hidden. Confirm this change is intentional. - Reason this comment was not posted:
Comment looked like it was already resolved.
Workflow ID: wflow_dK0ROblRNhjyFsOf
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on e83967a in 1 minute and 10 seconds
More details
- Looked at
69lines of code in1files - Skipped
0files when reviewing. - Skipped posting
6drafted comments based on config settings.
1. apps/web/client/src/app/project/[id]/_components/editor-bar/inputs/input-icon.tsx:21
- Draft comment:
Consider adding a runtime check to ensure Icons[icon] exists to avoid potential undefined renders. - Reason this comment was not posted:
Confidence changes required:50%<= threshold50%
None
2. apps/web/client/src/app/project/[id]/_components/editor-bar/inputs/input-icon.tsx:33
- Draft comment:
Using the initial prop value for input state may lead to stale state if the prop changes externally. Consider syncing state with prop changes. - Reason this comment was not posted:
Confidence changes required:50%<= threshold50%
None
3. apps/web/client/src/app/project/[id]/_components/editor-bar/inputs/input-icon.tsx:1
- Draft comment:
PR description mismatch: The code changes focus on the InputIcon component’s icon functionality, but the issue mentions improving onboarding docs. Ensure that the required documentation updates are included. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%
The comment is asking the PR author to ensure that documentation updates are included, which violates the rule against asking the author to update the PR description or ensure certain actions are taken. The comment does not provide a specific code suggestion or ask for a specific test to be written. Therefore, it should be removed.
4. apps/web/client/src/app/project/[id]/_components/editor-bar/inputs/input-icon.tsx:52
- Draft comment:
Consider adding fallback handling or a default icon if Icons[icon] is undefined to avoid silent failures. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%
None
5. apps/web/client/src/app/project/[id]/_components/editor-bar/inputs/input-icon.tsx:58
- Draft comment:
Improve accessibility by adding an aria-label to the input element. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%
None
6. apps/web/client/src/app/project/[id]/_components/editor-bar/inputs/input-icon.tsx:60
- Draft comment:
Review the numeric input validation—using pattern '[0-9]*' may restrict valid numeric formats (e.g., negatives or decimals) if needed. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%
None
Workflow ID: wflow_9Kl7HEaVahR4q7fs
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on e26bd22 in 1 minute and 5 seconds
More details
- Looked at
59lines of code in1files - Skipped
0files when reviewing. - Skipped posting
5drafted comments based on config settings.
1. apps/web/client/src/app/project/[id]/_components/editor-bar/inputs/input-icon.tsx:56
- Draft comment:
Icon size increased from h-4/w-4 to h-5/w-5 with added min sizes. Confirm this is intentional as it affects layout. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%
None
2. apps/web/client/src/app/project/[id]/_components/editor-bar/inputs/input-icon.tsx:57
- Draft comment:
DropdownMenu has been moved inside the styled container alongside the input. Verify that the new structure meets the intended design alignment. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%
None
3. apps/web/client/src/app/project/[id]/_components/editor-bar/inputs/input-icon.tsx:55
- Draft comment:
Icon container repositioning: The icon is now rendered outside the background container. Please verify that this change aligns with the intended design. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%
None
4. apps/web/client/src/app/project/[id]/_components/editor-bar/inputs/input-icon.tsx:56
- Draft comment:
Icon styling update: The icon size has been increased (h-5, w-5) with added min dimensions. Ensure these changes are consistent with the overall UI design. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%
None
5. apps/web/client/src/app/project/[id]/_components/editor-bar/inputs/input-icon.tsx:68
- Draft comment:
Dropdown menu alignment: The dropdown remains inside the input's background container. Confirm that its alignment works well with the new icon positioning. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%
None
Workflow ID: wflow_V80BZqxLVfp92rx1
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
There was a problem hiding this comment.
❌ Changes requested. Incremental review on 66cd27d in 2 minutes and 0 seconds
More details
- Looked at
37lines of code in1files - Skipped
0files when reviewing. - Skipped posting
5drafted comments based on config settings.
1. apps/web/client/src/app/project/[id]/_components/editor-bar/inputs/input-dropdown.tsx:40
- Draft comment:
Both unit-select and dropdown button use the same onDropdownChange callback. This can lead to confusion if different actions are intended. Consider separating these callbacks. - 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. apps/web/client/src/app/project/[id]/_components/editor-bar/inputs/input-dropdown.tsx:12
- Draft comment:
Consider parameterizing the unit options instead of using a hard-coded UNITS constant if customization is needed. - Reason this comment was not posted:
Confidence changes required:50%<= threshold50%
None
3. apps/web/client/src/app/project/[id]/_components/editor-bar/inputs/input-dropdown.tsx:1
- Draft comment:
PR description mismatch: The PR claims to address onboarding docs, but this change only updates the UI component. Please ensure the documentation changes are included or the issue reference is corrected. - 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. apps/web/client/src/app/project/[id]/_components/editor-bar/inputs/input-dropdown.tsx:41
- Draft comment:
Accessibility improvement: Consider adding ARIA attributes (e.g., aria-label) to the DropdownMenuTrigger to enhance accessibility. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%
None
5. apps/web/client/src/app/project/[id]/_components/editor-bar/inputs/input-dropdown.tsx:45
- Draft comment:
Minor cleanup: The explicit type annotation in the map callback (unitOption: string) is redundant given that UNITS is already typed as a string array. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%
None
Workflow ID: wflow_9S8myrbsu5cgvaQ2
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.
| {UNITS.map((unitOption: string) => ( | ||
| <DropdownMenuItem | ||
| key={unitOption} | ||
| onClick={() => onDropdownChange?.(unitOption)} |
There was a problem hiding this comment.
Ambiguous callback: onDropdownChange is used for both the unit dropdown and the second dropdown. Consider splitting into separate callbacks (e.g., onUnitChange and onOptionChange) for clearer intent.
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on 5bfeba2 in 1 minute and 52 seconds
More details
- Looked at
49lines of code in1files - Skipped
0files when reviewing. - Skipped posting
6drafted comments based on config settings.
1. apps/web/client/src/app/project/[id]/_components/editor-bar/inputs/input-range.tsx:15
- Draft comment:
Consider consistent casing: default unit is 'px' but options are uppercase (e.g., 'PX'). - Reason this comment was not posted:
Comment was on unchanged code.
2. apps/web/client/src/app/project/[id]/_components/editor-bar/inputs/input-range.tsx:91
- Draft comment:
Ensure DropdownMenuTrigger is accessible by adding ARIA attributes if needed. - 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%
Since this is using an internal UI library component (DropdownMenuTrigger), it's very likely that accessibility concerns are already handled by the component itself. Most modern UI libraries handle ARIA attributes internally. Without seeing the implementation of @onlook/ui-v4/dropdown-menu, we can't be certain that this attribute is actually needed or wouldn't conflict with existing accessibility handling.
The comment could be correct if the internal UI library doesn't handle accessibility properly. Poor accessibility is a real issue that should be addressed.
While accessibility is important, suggesting ARIA attributes for a component from an internal UI library without knowledge of its implementation could cause conflicts or redundancy. The proper place to handle this would be in the UI library itself.
Delete this comment as it makes assumptions about the internal implementation of a UI library component and could potentially conflict with existing accessibility handling.
3. apps/web/client/src/app/project/[id]/_components/editor-bar/inputs/input-range.tsx:91
- Draft comment:
Review indentation and formatting for the DropdownMenu block for readability. - Reason this comment was not posted:
Confidence changes required:30%<= threshold50%
None
4. apps/web/client/src/app/project/[id]/_components/editor-bar/inputs/input-range.tsx:15
- Draft comment:
Default unit value is set to 'px' (lowercase) while the dropdown options (UNITS) are uppercase ('PX', '%', etc.). Consider aligning the casing for consistency. - Reason this comment was not posted:
Comment was on unchanged code.
5. apps/web/client/src/app/project/[id]/_components/editor-bar/inputs/input-range.tsx:92
- Draft comment:
For accessibility, consider adding aria-labels or other appropriate attributes on the DropdownMenuTrigger. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%
None
6. apps/web/client/src/app/project/[id]/_components/editor-bar/inputs/input-range.tsx:1
- Draft comment:
The PR description mentions improvements to onboarding docs, but the changes here relate to UI component behavior. Please ensure that documentation updates are included if intended to fully resolve the issue. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%
This comment is asking the PR author to ensure that documentation updates are included, which violates the rule against asking the author to ensure things are tested or confirmed. It also indirectly asks for the PR description to be updated, which is not allowed.
Workflow ID: wflow_476vfNBEJHUUvXVX
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on 9db2b7a in 1 minute and 48 seconds
More details
- Looked at
15lines of code in1files - Skipped
0files when reviewing. - Skipped posting
8drafted comments based on config settings.
1. packages/ui-v4/src/components/icons/index.tsx:1919
- Draft comment:
The PR description and issue mention improved onboarding docs, but this diff only touches the Icons module. Please ensure that the changes actually address the intended onboarding documentation. - 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:132
- Draft comment:
This file is quite large with many SVG components defined inline. Consider splitting it into smaller, more manageable modules for maintainability and easier future updates. - 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:1846
- Draft comment:
Many SVG paths are styled inline (using fill/color attributes), which may be difficult to theme consistently. Consider using shared CSS classes or theme variables where appropriate. - Reason this comment was not posted:
Comment was on unchanged code.
4. packages/ui-v4/src/components/icons/index.tsx:132
- Draft comment:
Although icons are for UI display, consider adding ARIA attributes (or setting aria-hidden=true) for accessibility, ensuring decorative icons do not clutter the accessibility tree. - 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. packages/ui-v4/src/components/icons/index.tsx:1846
- Draft comment:
The PR description claims it resolves issue 123 for adding better onboarding docs, but this diff only shows icon component definitions with no documentation updates. Please confirm that the onboarding docs are included. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%
This comment is asking the PR author to confirm that the onboarding docs are included, which violates the rule against asking the author to confirm their intention or to update the PR description. It does not provide a specific code suggestion or request for tests.
6. packages/ui-v4/src/components/icons/index.tsx:1919
- Draft comment:
This file aggregates a huge number of SVG icon components. Consider splitting these into multiple modules to improve maintainability and enable better tree-shaking. - 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.
7. packages/ui-v4/src/components/icons/index.tsx:1848
- Draft comment:
Several SVG elements redundantly apply fill attributes both via the fill prop and inline style (e.g. style={{ fill: '#929292', fillOpacity: 1 }}). Consider removing redundant styling for clarity. - Reason this comment was not posted:
Comment was on unchanged code.
8. packages/ui-v4/src/components/icons/index.tsx:1850
- Draft comment:
For accessibility, consider adding aria-hidden="true" (or appropriate aria attrs) to these SVG icons if they are purely decorative. - 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_fMS29t8Mt0TH6rB1
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
ml-delaurier
pushed a commit
to ml-delaurier/nolook
that referenced
this pull request
Apr 23, 2025
* Bucket and background * Fixed border icon * Updated components * Updated bucket, border, image styling * Lots updated * Display component * Input components - radio added * Layout & position panel styling * New layout components and styling * Updated display types * Position fixed * Spacing section added * Update editor bar right side * Typography panel base * Typography values * Updated unit dropdown * Color input * Added color input - updated styles * Panel buttons styled * Update index.tsx * Fixed errors * Update input-icon.tsx * Update input-dropdown.tsx * Update input-range.tsx * fix icons kebab case
zongkelong
pushed a commit
to zongkelong/coolook
that referenced
this pull request
Apr 25, 2025
* 'main' of https://github.com/onlook-dev/onlook: use DomElementStyles (onlook-dev#1810) feat: Added file watcher & auto update modified opened files in Dev Tab (onlook-dev#1802) fix: border thickness hidden when setting border color from color picker (onlook-dev#1805) move overflow input (onlook-dev#1800) Added overflow option in the style panel (onlook-dev#1793) refactor: update images manager (onlook-dev#1784) chore: error nesting button (onlook-dev#1790) refactor: update naming for brand color (onlook-dev#1785) Tys toolbar v4 (onlook-dev#1786)
t1c1
pushed a commit
to t1c1/onlookbotcodes
that referenced
this pull request
Jun 5, 2025
* Bucket and background * Fixed border icon * Updated components * Updated bucket, border, image styling * Lots updated * Display component * Input components - radio added * Layout & position panel styling * New layout components and styling * Updated display types * Position fixed * Spacing section added * Update editor bar right side * Typography panel base * Typography values * Updated unit dropdown * Color input * Added color input - updated styles * Panel buttons styled * Update index.tsx * Fixed errors * Update input-icon.tsx * Update input-dropdown.tsx * Update input-range.tsx * fix icons kebab case
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Related Issues
Type of Change
Testing
Screenshots (if applicable)
Additional Notes
Important
Refactor editor bar components for modularity and update dependencies in
bun.lock.div-selected.tsxandimg-selected.tsxto use new componentsDisplay,Border,ColorBackground,ImageBackground,ImgFit, andViewButtons.Display,Border,ColorBackground,ImageBackground,ImgFit,ViewButtons,InputColor,InputRadio,InputIcon,InputRange, andPanels.EditorBarto acceptselectedElementprop.apps/backendentry frombun.lock.agent-baseandhttps-proxy-agentdependencies inbun.lock.This description was created by
for d4c3c3c. It will automatically update as commits are pushed.