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

Navigation drawer scroll padding fix #9141

Merged
Merged
Show file tree
Hide file tree
Changes from 16 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,9 @@ import styled from '@emotion/styled';
const StyledMainSection = styled(NavigationDrawerSection)`
min-height: fit-content;
`;
const StyledInnerContainer = styled.div`
height: 100%;
`;

export const MainNavigationDrawerItems = () => {
const isMobile = useIsMobile();
Expand Down Expand Up @@ -60,12 +63,14 @@ export const MainNavigationDrawerItems = () => {
contextProviderName="navigationDrawer"
componentInstanceId={`scroll-wrapper-navigation-drawer`}
defaultEnableXScroll={false}
scrollHide={true}
scrollbarVariant="no-padding"
>
<NavigationDrawerOpenedSection />
<CurrentWorkspaceMemberFavoritesFolders />
<WorkspaceFavorites />
<RemoteNavigationDrawerSection />
<StyledInnerContainer>
<NavigationDrawerOpenedSection />
<CurrentWorkspaceMemberFavoritesFolders />
<WorkspaceFavorites />
<RemoteNavigationDrawerSection />
</StyledInnerContainer>
</ScrollWrapper>
</>
);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,21 @@ const StyledTableFoot = styled.thead<{ endOfTableSticky?: boolean }>`
tr {
position: sticky;
z-index: 5;
${({ endOfTableSticky }) => endOfTableSticky && `bottom: 0;`}
background: ${({ theme }) => theme.background.primary};
ehconitin marked this conversation as resolved.
Show resolved Hide resolved
${({ endOfTableSticky }) =>
endOfTableSticky &&
`
bottom: 10px;
&::after {
content: '';
position: absolute;
bottom: -10px;
left: 0;
right: 0;
height: 10px;
background: inherit;
}
`}
}
`;

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import { useIsMobile } from '@/ui/utilities/responsive/hooks/useIsMobile';
import { NAV_DRAWER_WIDTHS } from '@/ui/navigation/navigation-drawer/constants/NavDrawerWidths';

import { useIsSettingsDrawer } from '@/navigation/hooks/useIsSettingsDrawer';
import { NavigationDrawerSection } from '@/ui/navigation/navigation-drawer/components/NavigationDrawerSection';
import { isNavigationDrawerExpandedState } from '../../states/isNavigationDrawerExpanded';
import { NavigationDrawerBackButton } from './NavigationDrawerBackButton';
import { NavigationDrawerHeader } from './NavigationDrawerHeader';
Expand Down Expand Up @@ -43,7 +44,7 @@ const StyledContainer = styled.div<{
? theme.spacing(3, 8)
: theme.spacing(3, 8, 4, 0)
: theme.spacing(3, 2, 4)};

padding-right: 0px;
@media (max-width: ${MOBILE_VIEWPORT}px) {
width: 100%;
padding-left: 20px;
Expand Down Expand Up @@ -123,7 +124,7 @@ export const NavigationDrawer = ({
<StyledItemsContainer isSettings={isSettingsDrawer}>
{children}
</StyledItemsContainer>
{footer}
<NavigationDrawerSection>{footer}</NavigationDrawerSection>
</StyledContainer>
</StyledAnimatedContainer>
);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -92,7 +92,7 @@ const StyledItem = styled('button', {
width: ${(props) =>
!props.isNavigationDrawerExpanded
? `calc(${NAV_DRAWER_WIDTHS.menu.desktop.collapsed}px - ${props.theme.spacing(5.5)})`
: `calc(100% - ${props.theme.spacing(2)})`};
: `calc(100% - ${props.theme.spacing(1.5)})`};

${({ isDragging }) =>
isDragging &&
Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import styled from '@emotion/styled';
import { useIsMobile } from 'twenty-ui';

const StyledSection = styled.div`
display: flex;
Expand All @@ -9,4 +10,25 @@ const StyledSection = styled.div`
flex-shrink: 1;
`;

export { StyledSection as NavigationDrawerSection };
const StyledSectionInnerContainerMinusScrollPadding = styled.div<{
isMobile: boolean;
}>`
width: calc(
100% - ${({ isMobile, theme }) => (isMobile ? 0 : theme.spacing(2))}
);
`;

export const NavigationDrawerSection = ({
Copy link
Member

Choose a reason for hiding this comment

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

I don't understand all these changes? Looks big changes for a small fix, could you explain it a bit more?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We can't just remove scrollHide and use the ScrollWrapper in the drawer since it overlaps over the NavigationDrawerItems(for reference #9026), so the idea is to remove the right padding of the NavigationDrawers animated container. And since NavigationDrawerSection controls the width, have an internal wrapper that would minus the width required for the scroll.
Is this the right approach? also, ignore the scroll paddings and widths being changed in ScrollWrapper, I was just trying something out!

children,
}: {
children: React.ReactNode;
}) => {
const isMobile = useIsMobile();
return (
<StyledSection>
<StyledSectionInnerContainerMinusScrollPadding isMobile={isMobile}>
{children}
</StyledSectionInnerContainerMinusScrollPadding>
</StyledSection>
);
};
Original file line number Diff line number Diff line change
Expand Up @@ -13,13 +13,14 @@ import { scrollWrapperInstanceComponentState } from '@/ui/utilities/scroll/state
import { scrollWrapperScrollLeftComponentState } from '@/ui/utilities/scroll/states/scrollWrapperScrollLeftComponentState';
import { scrollWrapperScrollTopComponentState } from '@/ui/utilities/scroll/states/scrollWrapperScrollTopComponentState';
import { useSetRecoilComponentStateV2 } from '@/ui/utilities/state/component-state/hooks/useSetRecoilComponentStateV2';
import { css } from '@emotion/react';
import 'overlayscrollbars/overlayscrollbars.css';

type HeightMode = 'full' | 'fit-content';

const StyledScrollWrapper = styled.div<{
scrollHide?: boolean;
heightMode: HeightMode;
scrollbarVariant: 'with-padding' | 'no-padding';
}>`
display: flex;
height: ${({ heightMode }) => {
Expand All @@ -33,9 +34,38 @@ const StyledScrollWrapper = styled.div<{
width: 100%;

.os-scrollbar-handle {
background-color: ${({ theme, scrollHide }) =>
scrollHide ? 'transparent' : theme.border.color.medium};
background-color: ${({ theme }) => theme.border.color.strong};
}

// Keep horizontal scrollbar always visible
.os-scrollbar-horizontal {
&.os-scrollbar-auto-hide {
opacity: 1;
visibility: visible;
}
.os-scrollbar-track {
visibility: visible !important;
}
}

.os-scrollbar {
transition:
opacity 300ms,
visibility 300ms,
top 300ms,
right 300ms,
bottom 300ms,
left 300ms;
}

${({ scrollbarVariant }) =>
scrollbarVariant === 'no-padding' &&
css`
.os-scrollbar {
--os-size: 6px;
padding: 0px;
}
`}
`;

const StyledInnerContainer = styled.div`
Expand All @@ -49,8 +79,8 @@ export type ScrollWrapperProps = {
defaultEnableXScroll?: boolean;
defaultEnableYScroll?: boolean;
contextProviderName: ContextProviderName;
scrollHide?: boolean;
componentInstanceId: string;
scrollbarVariant?: 'with-padding' | 'no-padding';
};

export const ScrollWrapper = ({
Expand All @@ -61,7 +91,7 @@ export const ScrollWrapper = ({
defaultEnableXScroll = true,
defaultEnableYScroll = true,
contextProviderName,
scrollHide = false,
scrollbarVariant = 'with-padding',
}: ScrollWrapperProps) => {
const scrollableRef = useRef<HTMLDivElement>(null);
const Context = getContextByProviderName(contextProviderName);
Expand Down Expand Up @@ -94,49 +124,31 @@ export const ScrollWrapper = ({
autoHideDelay: 500,
},
overflow: {
x: defaultEnableXScroll ? 'scroll' : 'hidden',
y: defaultEnableYScroll ? 'scroll' : 'hidden',
x: defaultEnableXScroll ? undefined : 'hidden',
y: defaultEnableYScroll ? undefined : 'hidden',
},
},
events: {
scroll: (osInstance) => {
const {
scrollOffsetElement: target,
scrollbarHorizontal,
scrollbarVertical,
} = osInstance.elements();

// Hide scrollbars by default
[scrollbarHorizontal, scrollbarVertical].forEach((scrollbar) => {
if (scrollbar !== null) {
scrollbar.track.style.visibility = 'hidden';
}
});

// Show appropriate scrollbar based on scroll direction
const isHorizontalScroll =
target.scrollLeft !== Number(target.dataset.lastScrollLeft || '0');
const { scrollOffsetElement: target, scrollbarVertical } =
osInstance.elements();
// Hide vertical scrollbar by default
if (scrollbarVertical !== null) {
scrollbarVertical.track.style.visibility = 'hidden';
}

// Show vertical scrollbar based on scroll direction
const isVerticalScroll =
target.scrollTop !== Number(target.dataset.lastScrollTop || '0');

// Show scrollbar based on scroll direction only with explicit conditions
if (
isHorizontalScroll === true &&
scrollbarHorizontal !== null &&
target.scrollWidth > target.clientWidth
) {
scrollbarHorizontal.track.style.visibility = 'visible';
}
if (
isVerticalScroll === true &&
scrollbarVertical !== null &&
target.scrollHeight > target.clientHeight
) {
scrollbarVertical.track.style.visibility = 'visible';
}

// Update scroll positions
target.dataset.lastScrollLeft = target.scrollLeft.toString();
// Update vertical scroll positions
target.dataset.lastScrollTop = target.scrollTop.toString();

handleScroll(osInstance);
Expand All @@ -146,19 +158,16 @@ export const ScrollWrapper = ({

useEffect(() => {
const currentRef = scrollableRef.current;

if (currentRef !== null) {
initialize(currentRef);
}

return () => {
// Reset all component-specific Recoil state
// Reset vertical scroll component-specific Recoil state
setScrollTop(0);
setScrollLeft(0);
setOverlayScrollbars(null);
instance()?.destroy();
};
}, [initialize, instance, setScrollTop, setScrollLeft, setOverlayScrollbars]);
}, [initialize, instance, setScrollTop, setOverlayScrollbars]);

useEffect(() => {
setOverlayScrollbars(instance());
Expand All @@ -177,8 +186,8 @@ export const ScrollWrapper = ({
<StyledScrollWrapper
ref={scrollableRef}
className={className}
Copy link
Member

Choose a reason for hiding this comment

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

why are we removing the scrollHide?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We use scrollHide specifically to hide NavigationDrawer's scroll, but we have new requirements :). We are to show the scroll but with clear paddings

scrollHide={scrollHide}
heightMode={heightMode}
scrollbarVariant={scrollbarVariant}
>
<StyledInnerContainer>{children}</StyledInnerContainer>
</StyledScrollWrapper>
Expand Down
Loading