-
Notifications
You must be signed in to change notification settings - Fork 1.3k
feat: Allow user to add a window from the bottom toolbar #2113
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
base: main
Are you sure you want to change the base?
Conversation
@Paribesh01 is attempting to deploy a commit to the Onlook Team on Vercel. A member of the Team first needs to authorize it. |
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.
Important
Looks good to me! 👍
Reviewed everything up to c88dade in 1 minute and 22 seconds. Click for details.
- Reviewed
240
lines of code in8
files - Skipped
0
files when reviewing. - Skipped posting
3
draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. apps/web/client/src/app/project/[id]/_components/bottom-bar/index.tsx:68
- Draft comment:
The addWindowFunc duplicates the currently selected window. Confirm that ‘adding a window’ should duplicate an existing window rather than creating a fresh one. - 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 comment violates the rule about not asking authors to confirm their intentions. The code is clear and intentional - the button is disabled when no window is selected, showing that duplication is the intended behavior. The name "addWindow" is broad enough to encompass duplication as a form of adding. Perhaps there's a legitimate UX concern about the button name not matching its behavior exactly. The function name could be misleading to users. The UI/UX concerns are outside our scope per the rules. We should trust that the author has considered the user experience and chosen these names intentionally. Delete this comment as it merely asks for confirmation of intended behavior, which violates our review rules.
2. apps/web/client/src/app/project/[id]/_components/bottom-bar/index.tsx:128
- Draft comment:
The add window button is disabled when no frame is selected. Ensure this is the intended UX; consider allowing creation of a new window even if no window is active. - 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 code shows that the "add window" feature is specifically designed to duplicate an existing frame, not create a new empty window. The disabled state is therefore correct and intentional since you can't duplicate nothing. The comment suggests changing the core functionality rather than pointing out a bug or improvement. I could be wrong about the intended functionality - maybe the duplicate behavior is a temporary implementation and it should eventually support creating new windows from scratch. Even if the functionality might change in the future, the current implementation is clearly intentional and the disabled state correctly matches the duplicate functionality. Delete this comment as it questions intentional behavior that correctly matches the implementation, and suggests a feature change rather than pointing out a bug or improvement.
3. apps/web/client/src/components/hotkey.ts:11
- Draft comment:
Addition of Hotkey.ADD_WINDOW is correct. Verify that the shortcut 'w' does not conflict with any other common browser/system shortcuts. - Reason this comment was not posted:
Confidence changes required:30%
<= threshold50%
None
Workflow ID: wflow_H56XbRqArmMmuzBO
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
@Kitenite can you plz review this and let me know if any change are required. |
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.
Hey @Paribesh01 , this is very nice already. What do you think about being able to draw in a window as well? Similar to insert but on the actual canvas?
It's a lot more challenge but since this PR is simple now what do you think about adding that? And how to handle if no window is currently selected?
@Kitenite ok, i will try my best and make the changes. |
Description
Added a feature where user can add a window from the bottom toolbar
Related Issues
#1910
Type of Change
Testing
try clicking a desktop icon on the bottom bar, it will create a new window
Screenshots (if applicable)
Additional Notes
Important
Add feature to allow users to add a window from the bottom toolbar with hotkey support and translations.
index.tsx
to allow users to add a window from the bottom toolbar.addWindowFunc
to duplicate selected frame and switch to WINDOWS tab.addWindow
hotkey (w
) inhotkey.ts
.addWindow
translations inen.d.json.ts
,en.json
,es.json
,ja.json
,ko.json
,zh.json
.This description was created by
for c88dade. You can customize this summary. It will automatically update as commits are pushed.