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: New Relation Design hot fix #7423

Merged
merged 4 commits into from
Oct 4, 2024
Merged

Conversation

harshit078
Copy link
Contributor

@harshit078 harshit078 commented Oct 3, 2024

## Description

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 implements responsive design changes for the relation settings form card in the data model settings, addressing issue #7353.

  • Added media queries in SettingsDataModelFieldRelationSettingsFormCard.tsx to create a mobile-specific layout
  • Implemented flex direction change to column for mobile screens using MOBILE_VIEWPORT constant
  • Added conditional image rotation for mobile view using the flip prop on StyledRelationImage
  • Retained original desktop layout while improving mobile display as requested

1 file(s) reviewed, 1 comment(s)
Edit PR Review Bot Settings

`;

const StyledRelationImage = styled.img<{ flip?: boolean }>`
transform: ${({ flip }) => (flip ? 'scaleX(-1) rotate(270deg)' : 'none')};
@media (max-width: ${MOBILE_VIEWPORT}px) {
transform: ${({ flip }) => (flip ? 'scaleX(-1) rotate(450deg)' : 'none')};
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 a CSS transform function for better performance

@ehconitin ehconitin self-assigned this Oct 4, 2024
@ehconitin
Copy link
Contributor

@harshit078 its still broken, Ill suggest to use useIsMobile hook instead!

2024-10-04.17-20-21.mp4

@ehconitin
Copy link
Contributor

@Bonapara we should merge this right? since its broken on main branch

2024-10-04.17-25-41.mp4

Copy link
Contributor

@ehconitin ehconitin left a comment

Choose a reason for hiding this comment

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

LGTM!

@ehconitin
Copy link
Contributor

@FelixMalfait could u re-review please!
Thanks

@Bonapara
Copy link
Member

Bonapara commented Oct 4, 2024

On mobile, we could display the relation type above the object destination to prevent the text from being cropped.

03c148e24a1fea2d5a1e71a1d4eb3a1a0202bfd75e521f80da3c90909a6b79d7

@ehconitin
Copy link
Contributor

Ah, missed it!
Its a quick fix

Copy link
Member

@FelixMalfait FelixMalfait left a comment

Choose a reason for hiding this comment

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

Thank you!

@FelixMalfait FelixMalfait merged commit 424c489 into twentyhq:main Oct 4, 2024
4 of 8 checks passed
Copy link

oss-gg bot commented Oct 4, 2024

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

Copy link

github-actions bot commented Oct 4, 2024

Thanks @harshit078 for your contribution!
This marks your 22nd PR on the repo. You're top 2% of all our contributors 🎉
See contributor page - Share on LinkedIn - Share on Twitter

Contributions

harshit078 added a commit to harshit078/twenty that referenced this pull request Oct 14, 2024
## Description

- This PR solves the issue twentyhq#7353 

- [x]  Improved layout for mobile and desktop

-    [ ] Added tooltip on hover

---------

Co-authored-by: Nitin Koche <[email protected]>
Co-authored-by: Félix Malfait <[email protected]>
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.

4 participants