Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Debugged Properties panel close Animation Bug #654

Merged
merged 5 commits into from
Oct 25, 2024
Merged

Debugged Properties panel close Animation Bug #654

merged 5 commits into from
Oct 25, 2024

Conversation

PrakharJain1509
Copy link
Contributor

Fixing Overflow Issue in EditPanel Component

Problem

While opening or closing the EditPanel tab, the content inside was overflowing beyond the boundaries of the panel. This caused the UI to break during animations.

Solution

We applied overflow-hidden to critical containers and added width/height constraints to prevent content from escaping during transitions. We also ensured smooth transitions with CSS adjustments.

Changes Made

  1. Apply overflow-hidden to the Main Wrapper:
    • Ensures that the content stays within the panel boundaries during open/close transitions.
    <div
    className={clsx(
        'fixed right-0 transition-width duration-300 opacity-100 bg-background/80 rounded-tl-xl overflow-hidden',
        editorEngine.mode === EditorMode.INTERACT ? 'hidden' : 'visible',
        !isOpen && 'w-12 h-12 rounded-l-xl cursor-pointer',
        isOpen && 'h-[calc(100vh-5rem)]',
        isOpen && selectedTab == TabValue.STYLES && 'w-60',
        isOpen && selectedTab == TabValue.CHAT && 'w-[22rem]',
    )}
>

@PrakharJain1509
Copy link
Contributor Author

@drfarrell @Kitenite here is the PR for that same, This has successfully debugged that issue and the text is now not coming out of the transition box. Please review it and let me know if any changes are required.

@PrakharJain1509
Copy link
Contributor Author

This was for #651 .

Kitenite
Kitenite requested changes Oct 25, 2024
View reviewed changes
Copy link
Contributor

@Kitenite Kitenite left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is certainly better! However, I'm seeing some regression from this change:

  1. the tailwind class box is larger than it should be.
  2. The editor panel no longer scrolls
Screenshot 2024-10-25 at 4 16 34 PM

@PrakharJain1509 PrakharJain1509 mentioned this pull request Oct 25, 2024
Closed
@PrakharJain1509
Copy link
Contributor Author

PrakharJain1509 commented Oct 25, 2024 •
edited
Loading

@Kitenite Please check now, Scroll is working now, and the tailwind class is also in size..

@PrakharJain1509
Copy link
Contributor Author

@Kitenite Any updates sir?

Kitenite
Kitenite reviewed Oct 25, 2024
View reviewed changes
Copy link
Contributor

@Kitenite Kitenite left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just one question, seems like there are more overflow-hidden implemented in code than what was suggested in the description. Is this correct?
Also should we do the same thing in the LayersPanel?

app/src/routes/editor/EditPanel/StylesTab/index.tsx Outdated
@@ -104,7 +104,7 @@ const ManualTab = observer(() => {
<AccordionTrigger>
<h2 className="text-xs font-semibold">{groupKey}</h2>
</AccordionTrigger>
<AccordionContent>
<AccordionContent className="overflow-auto max-h-[50vh]">
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you explain why this is necessary? Seems like we just need overflow-hidden on the root panel div?

app/src/routes/editor/EditPanel/StylesTab/index.tsx Outdated
@@ -128,7 +128,7 @@ const ManualTab = observer(() => {
return (
editorEngine.elements.selected.length > 0 && (
<Accordion
className="px-4"
className="px-4 overflow-hidden"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same with this

app/src/routes/editor/EditPanel/index.tsx Outdated
<div className="h-[calc(100vh-7.75rem)] overflow-auto">
<TabsContent value={TabValue.STYLES}>
<TabsContent value={TabValue.STYLES} className="min-w-0 max-w-full min-h-0 max-h-full">
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And these 2

@Kitenite
Copy link
Contributor

@Kitenite Any updates sir?

Sorry I was working on a different PR. Left some comments but thanks for the quick turnaround!

@PrakharJain1509
Copy link
Contributor Author

@Kitenite Any updates sir?

Sorry I was working on a different PR. Left some comments but thanks for the quick turnaround!

So I was not sure where the code was at first so was experimenting, So is it fine now ?

@PrakharJain1509
Copy link
Contributor Author

@Kitenite Did it for the layers too.

Kitenite
Kitenite approved these changes Oct 25, 2024
View reviewed changes
Copy link
Contributor

@Kitenite Kitenite left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Working well! Thanks for being very responsive on this

@Kitenite Kitenite merged commit b0fc030 into onlook-dev:main Oct 25, 2024
@PrakharJain1509
Copy link
Contributor Author

@Kitenite Can you please add the hacktoberfest-accepted label also, for it to get counted.

@Kitenite
Copy link
Contributor

Ofc!

@Kitenite Kitenite linked an issue Oct 25, 2024 that may be closed by this pull request
[bug] Properties panel close animation is slightly buggy #651
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Kitenite Kitenite Kitenite approved these changes

Assignees
No one assigned
Labels
hacktoberfest-accepted
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[bug] Properties panel close animation is slightly buggy
2 participants
@PrakharJain1509 @Kitenite