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

Fix pricing modal being cut off and unscrollabel on low resolution screens or when zoomed in #8848

Merged
merged 10 commits into from
Dec 17, 2024
Merged

Fix pricing modal being cut off and unscrollabel on low resolution screens or when zoomed in #8848

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
4f4f9b2
Lift up justify-content: center to the parent container to avoid conf…
khuddite Dec 3, 2024
b9ead28
Merge branch 'main' into pricing-modal-padding
FelixMalfait Dec 6, 2024
ea5747e
Remove container?
FelixMalfait Dec 6, 2024
630c6d6
Forgot to commit this part
FelixMalfait Dec 6, 2024
56baa74
Better to use auto than scroll
FelixMalfait Dec 6, 2024
72f9dbb
Merge branch 'main' into pricing-modal-padding
FelixMalfait Dec 10, 2024
7df2ef5
Dont add scrollwrapper to import modal
FelixMalfait Dec 17, 2024
33e1c39
Fix
FelixMalfait Dec 17, 2024
6d80986
Merge branch 'main' into pricing-modal-padding
FelixMalfait Dec 17, 2024
46a82fc
Fix after merge
FelixMalfait Dec 17, 2024
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
8 changes: 7 additions & 1 deletion packages/twenty-front/src/modules/auth/components/AuthModal.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import { Modal } from '@/ui/layout/modal/components/Modal';
import { ScrollWrapper } from '@/ui/utilities/scroll/components/ScrollWrapper';
import styled from '@emotion/styled';
import React from 'react';

Expand All @@ -11,6 +12,11 @@ type AuthModalProps = { children: React.ReactNode };

export const AuthModal = ({ children }: AuthModalProps) => (
<Modal padding={'none'} modalVariant="primary">
<StyledContent>{children}</StyledContent>
<ScrollWrapper
contextProviderName="modalContent"
componentInstanceId="scroll-wrapper-modal-content"
>
<StyledContent>{children}</StyledContent>
</ScrollWrapper>
</Modal>
);
4 changes: 2 additions & 2 deletions packages/twenty-front/src/modules/ui/layout/modal/components/Modal.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,8 @@ const StyledModalDiv = styled(motion.div)<{
if (isMobile) return `0`;
return theme.border.radius.md;
}};
overflow: hidden;
overflow-x: hidden;
overflow-y: auto;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am not sure if there is a use case for this right now in the app, but here is what I am thinking.
When the modal has header, content, and footer, previously only content was scrollable meaning the header and footer will be sticky above/below the content. But now, the entire modal is scrollable, meaning the header or footer will be invisible at some point.

I am not sure if we are okay with following that path here. @FelixMalfait

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe that diminishes the purpose of the header/footer? IDK

z-index: 10000; // should be higher than Backdrop's z-index

width: ${({ isMobile, size, theme }) => {
Expand Down Expand Up @@ -79,7 +80,6 @@ const StyledContent = styled.div`
flex: 1;
flex: 1 1 0%;
flex-direction: column;
overflow-y: auto;
padding: ${({ theme }) => theme.spacing(10)};
`;

Expand Down
30 changes: 15 additions & 15 deletions packages/twenty-front/src/modules/ui/layout/page/components/DefaultLayout.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -126,24 +126,24 @@ export const DefaultLayout = () => {
) : (
<StyledAppNavigationDrawer />
)}
<StyledMainContainer>
{showAuthModal ? (
<>
<SignInBackgroundMockPage />
<AnimatePresence mode="wait">
<LayoutGroup>
<AuthModal>
<Outlet />
</AuthModal>
</LayoutGroup>
</AnimatePresence>
</>
) : (
{showAuthModal ? (
<>
<SignInBackgroundMockPage />
<AnimatePresence mode="wait">
<LayoutGroup>
<AuthModal>
<Outlet />
</AuthModal>
</LayoutGroup>
</AnimatePresence>
</>
) : (
<StyledMainContainer>
<AppErrorBoundary>
<Outlet />
</AppErrorBoundary>
)}
</StyledMainContainer>
</StyledMainContainer>
)}
</StyledPageContainer>
{isMobile && <MobileNavigationBar />}
</StyledLayout>
Expand Down
7 changes: 6 additions & 1 deletion packages/twenty-front/src/modules/ui/utilities/scroll/contexts/ScrollWrapperContexts.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,8 @@ export type ContextProviderName =
| 'releases'
| 'test'
| 'showPageActivityContainer'
| 'navigationDrawer';
| 'navigationDrawer'
| 'modalContent';

const createScrollWrapperContext = (id: string) =>
createContext<ScrollWrapperContextValue>({
Expand Down Expand Up @@ -51,6 +52,8 @@ export const ShowPageActivityContainerScrollWrapperContext =
export const NavigationDrawerScrollWrapperContext =
createScrollWrapperContext('navigationDrawer');
export const TestScrollWrapperContext = createScrollWrapperContext('test');
export const ModalContentScrollWrapperContext =
createScrollWrapperContext('modalContent');

export const getContextByProviderName = (
contextProviderName: ContextProviderName,
Expand Down Expand Up @@ -82,6 +85,8 @@ export const getContextByProviderName = (
return ShowPageActivityContainerScrollWrapperContext;
case 'navigationDrawer':
return NavigationDrawerScrollWrapperContext;
case 'modalContent':
return ModalContentScrollWrapperContext;
default:
throw new Error('Context Provider not available');
}
Expand Down
Loading