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.

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

[FIX] fix navigation overflow #7795

Merged
merged 5 commits into from
Oct 17, 2024
Merged

Conversation

Hitarthsheth07
Copy link
Contributor

FIX #7733

Fixes the overflow and responsive problem on large and small devices.

image
image

The 'Workspace' title is fixed and only links under it are scrolled when overflown.

Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

PR Summary

This pull request addresses the navigation overflow issue by implementing scrollable containers and adjusting styles in various components to enable vertical scrolling when items exceed the viewport height.

  • Added ObjectsMetaDataItemsWrapper with overflow-y: auto in NavigationDrawerSectionForObjectMetadataItems.tsx for vertical scrolling
  • Removed flex-grow: 1 from StyledNavigationDrawerItemContainer in NavigationDrawerItem.tsx to prevent items from expanding beyond screen size
  • Added max-height and overflow properties to StyledAnimatedContainer and StyledItemsContainer in NavigationDrawer.tsx
  • Implemented flex-shrink: 1 and overflow: hidden in NavigationDrawerSection.tsx to ensure proper scrolling behavior

5 file(s) reviewed, 6 comment(s)
Edit PR Review Bot Settings | Greptile

Comment on lines 25 to 33
const ObjectsMetaDataItemsWrapper = styled.div`
display: flex;
flex-direction: column;
gap: ${({ theme }) => theme.betweenSiblingsGap};
width: 100%;
margin-bottom: ${({ theme }) => theme.spacing(3)};
flex: 1;
overflow-y: auto;
`;
Copy link
Contributor

Choose a reason for hiding this comment

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

style: Consider adding a max-height property to ensure the container doesn't grow indefinitely

Comment on lines 150 to 152
<ObjectsMetaDataItemsWrapper>
{isNavigationSectionOpen && renderObjectMetadataItems()}
</ObjectsMetaDataItemsWrapper>
Copy link
Contributor

Choose a reason for hiding this comment

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

style: Ensure that the scrollable area doesn't interfere with other navigation elements when empty

@@ -44,7 +44,7 @@ export const NavigationDrawerAnimatedCollapseWrapper = ({
transition={{
duration: theme.animation.duration.normal,
}}
>
>
Copy link
Contributor

Choose a reason for hiding this comment

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

style: There's an extra space at the end of this line

@@ -6,6 +6,8 @@ const StyledSection = styled.div`
gap: ${({ theme }) => theme.betweenSiblingsGap};
width: 100%;
margin-bottom: ${({ theme }) => theme.spacing(3)};
flex-shrink: 1;
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.

syntax: Missing semicolon at the end of the CSS property

Comment on lines 9 to 10
flex-shrink: 1;
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.

style: Consider using overflow-y: auto instead of overflow: hidden to allow vertical scrolling while hiding horizontal overflow

@Hitarthsheth07
Copy link
Contributor Author

@lucasbordeau the merging was blocked for some reason.

Also, could you please assign appropriate OSS points to issue and PR

Cheers!

@charlesBochet
Copy link
Member

@Hitarthsheth07 you have lint issue (see CI) could you fix them?

@charlesBochet charlesBochet merged commit f6c094a into twentyhq:main Oct 17, 2024
11 of 13 checks passed
@charlesBochet
Copy link
Member

/award 300

Copy link

oss-gg bot commented Oct 17, 2024

Awarding Hitarthsheth07: 300 points 🕹️ Well done! Check out your new contribution on oss.gg/Hitarthsheth07

Copy link

Thanks @Hitarthsheth07 for your contribution!
This marks your 1st PR on the repo. You're top 51% of all our contributors 🎉
See contributor page - Share on LinkedIn - Share on Twitter

Contributions

@Hitarthsheth07
Copy link
Contributor Author

@charlesBochet the ci has failed for CI-front. Is there a issue on my side or a server problem. Thanks.

@charlesBochet
Copy link
Member

Thanks for checking it, it's great to see rigorous contributors :)

You are good, the failing CIs are failing because of infra issues

ijreilly added a commit that referenced this pull request Oct 30, 2024
In [this PR (fix navigation
overflow)](#7795) we introduced a
regression, hidding the left-side animation with advanced settings:
<img width="285" alt="Capture d’écran 2024-10-30 à 12 56 22"
src="https://github.com/user-attachments/assets/46d7b1e5-4759-42e9-9bcb-aaa0fedfe542">

<img width="274" alt="Capture d’écran 2024-10-30 à 12 56 31"
src="https://github.com/user-attachments/assets/d3c3d337-f6fc-4509-a920-4c2c7506f061">
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Navigation Bar extends beyond the screen size
3 participants