Skip to content

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

Merged
merged 18 commits into from
Apr 10, 2025
Merged

Tys toolbar v3 - halfway mark #1764

merged 18 commits into from
Apr 10, 2025

Conversation

thughey
Copy link
Contributor

@thughey thughey commented Apr 9, 2025

Description

Related Issues

Type of Change

  • Bug fix
  • New feature
  • Documentation update
  • Release
  • Refactor
  • Other (please describe):

Testing

Screenshots (if applicable)

Additional Notes


Important

Enhance toolbar with new dropdown components for styling and update icon set and package versions.

  • Components:
    • Add ImgSelected component in img-selected.tsx for image selection toolbar.
    • Add dropdown components Height, Width, Padding, Margin, Radius in respective files for styling options.
    • Update DivSelected in div-selected.tsx to include new dropdowns for layout options.
  • Icons:
    • Add CornerTopRight, CornerBottomRight, CornerBottomLeft icons in index.tsx.
    • Remove and re-add CornerTopRight, CornerBottomRight, CornerBottomLeft icons in index.tsx.
  • Misc:
    • Update EditorBar in index.tsx to include ImgSelected component.
    • Update bun.lock to reflect new package versions for @ai-sdk, dotenv, eslint, sharp, supabase, and others.

This description was created by Ellipsis for 75aa3c9. It will automatically update as commits are pushed.

Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a 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 in 16 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% <= threshold 50%
    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">
Copy link
Contributor

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">
Copy link
Contributor

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.

Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a 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 in 2 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% <= threshold 50%
    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% <= threshold 50%
    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% <= threshold 50%
    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% <= threshold 50%
    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% <= threshold 50%
    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.

@Kitenite Kitenite merged commit 8b5305c into main Apr 10, 2025
@Kitenite Kitenite deleted the tys-toolbar-v3 branch April 10, 2025 22:47
ml-delaurier pushed a commit to ml-delaurier/nolook that referenced this pull request Apr 23, 2025
* 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
t1c1 pushed a commit to t1c1/onlookbotcodes that referenced this pull request Jun 5, 2025
* 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants