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

Fixed Onboarding page as requested #Fixes 5740 #5758

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Fixed Onboarding page as requested #Fixes 5740 #5758

wants to merge 2 commits into from
+18 −0

Conversation

sagarhedaoo
Copy link
Contributor

Screenshot 2024-06-06 at 2 26 41 AM
Screenshot 2024-06-06 at 2 26 54 AM

Fixed the onboarding page as shown in figma design. if any further changes are required, I am happy to do that!

greptile-apps[bot]
greptile-apps bot reviewed Jun 6, 2024
View reviewed changes
Copy link

@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

  • Enhanced StyledContent in Modal.tsx for better alignment and scrolling
  • Added media queries in ModalLayout.tsx for mobile responsiveness

packages/twenty-front/src/modules/auth/components/Modal.tsx
@@ -6,6 +6,12 @@ import { ModalLayout } from '@/ui/layout/modal/components/ModalLayout';
const StyledContent = styled(ModalLayout.Content)`
align-items: center;
width: calc(400px - ${({ theme }) => theme.spacing(10 * 2)});
display: flex;
flex-direction: column;
Copy link

Choose a reason for hiding this comment

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

Duplicate 'align-items: center' property. Consider removing the redundant line.

FelixMalfait
FelixMalfait reviewed Jun 6, 2024
View reviewed changes
packages/twenty-front/src/modules/ui/layout/modal/components/ModalLayout.tsx
@@ -42,6 +42,18 @@ const StyledModalDiv = styled(motion.div)<{
return 'auto';
}
}};

@media (max-width: 768px) {
Copy link
Member

Choose a reason for hiding this comment

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

@media (max-width: ${MOBILE_VIEWPORT}px) {

@FelixMalfait
Copy link
Member

FelixMalfait commented Jun 6, 2024 •
edited
Loading

Thanks for the contribution. This doesn't look great for existing modales.

cc @Bonapara

Screenshot 2024-06-06 at 15 14 49

We probably want to differentiate when the modal can be closed VS when it cannot.
When it can be closed do we want a bottom sheet? or a close button @Bonapara?

@FelixMalfait
Copy link
Member

Also It's not properly centered on all screen sizes:

Screenshot 2024-06-06 at 15 20 38

@FelixMalfait
Copy link
Member

Screenshot 2024-06-06 at 15 22 01

@Bonapara
Copy link
Member

Bonapara commented Jun 6, 2024 •
edited
Loading

Thanks for the contribution. This doesn't look great for existing modales.

cc @Bonapara

Screenshot 2024-06-06 at 15 14 49 We probably want to differentiate when the modal can be closed VS when it cannot. When it can be closed do we want a bottom sheet? or a close button @Bonapara?

Could we keep the confirmation modal as it is today and make this PR affect only the onboarding flow?

Desired behavior for modal:

CleanShot 2024-06-06 at 15 31 28@2x

@Bonapara
Copy link
Member

Bonapara commented Jun 6, 2024 •
edited by FelixMalfait
Loading

Screenshot 2024-06-06 at 15 22 01

I think we can keep the desktop design on the tablet since the screen is already large enough. (However, we shouldn't change the current viewport definition for that)

Edit Félix: iPad Mini = phone size, so nothing to change indeed

@Bonapara
Copy link
Member

Bonapara commented Jun 6, 2024 •
edited
Loading

Hi @sagarhedaoo, thanks for taking on this issue!

Regarding what I said above, we should introduce the concept of "variants" for the modal to differentiate the onboarding from the in-app alerts.

Here are the specifications:

  Primary Secondary
Laptop Background #1B1B1B opacity 80% Background #1B1B1B opacity 40%
Mobile Fullscreen Background #1B1B1B opacity 40%
Ability to close by clicking outside or by pressing ⎋ (esc) Option (Can be activated or not) Option (Can be activated or not)

How does it sound?

@sagarhedaoo
Copy link
Contributor Author

@Bonapara
Variants can be interesting to try! Do you want me to implement variants for the modal?

greptile-apps[bot]
greptile-apps bot reviewed Jun 6, 2024
View reviewed changes
Copy link

@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

(updates since last review)

  • Added inverted prop to Button component for alternative styling
  • Updated Button stories to include inverted prop for testing
  • Removed unique position check in RecordPositionFactory
  • Adjusted tests in record-position.factory.spec.ts to align with new logic
  • Introduced optional backgroundColor prop in ComponentStorybookLayout for customizable styling

@FelixMalfait
Copy link
Member

FelixMalfait commented Jun 7, 2024 •
edited
Loading

Yes that'd be great, let's try to do something clean that fits all use-cases :). Thanks!

@lucasbordeau lucasbordeau self-assigned this Jun 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@FelixMalfait FelixMalfait FelixMalfait left review comments

@greptile-apps greptile-apps[bot] greptile-apps left review comments

At least 1 approving review is required to merge this pull request.

Assignees

@lucasbordeau lucasbordeau

Labels
PR: awaiting author
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@sagarhedaoo @FelixMalfait @Bonapara @lucasbordeau