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: Invite by email table overflows in mobile viewport #7273

Merged
merged 18 commits into from
Oct 8, 2024

Conversation

harshit078
Copy link
Contributor

@harshit078 harshit078 commented Sep 26, 2024

##Description

Before

Screen.Recording.2024-09-27.at.1.30.14.AM.mov
Screenshot 2024-09-27 at 1 30 52 AM

After

Screenshot 2024-09-27 at 1 34 11 AM
Screen.Recording.2024-09-27.at.1.34.39.AM.mov

Note

I've added 2 implementations and if either doesn't follow design rules then it can be changed-

  • Made the trash icon accent danger
  • When emails are long, given scroll for ease of convience.

@harshit078 harshit078 changed the title fix: Improved layout and made it mobile friendly fix: Invite by email table overflows in mobile viewport Sep 26, 2024
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 improves the layout of the workspace members settings page, focusing on mobile responsiveness and addressing table overflow issues.

  • Introduced StyledTableRow and StyledTableCell components with mobile-specific styling in SettingsWorkspaceMembers.tsx
  • Added media query for mobile viewport to adjust table layout on smaller screens
  • Modified StyledTableCell to handle long email inputs by adding overflow: scroll and text-overflow: ellipsis
  • Adjusted gridAutoColumns for StyledTableRow to improve spacing on mobile devices
  • Implemented these changes to solve the table overflow issue reported in Invite by email table overflows in mobile viewport #7253

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

@ehconitin
Copy link
Contributor

Note

I've added 2 implementations and if either doesn't follow design rules then it can be changed-

  • Made the trash icon accent danger
  • When emails are long, given scroll for ease of convience.

@Bonapara

@Bonapara
Copy link
Member

The video won't play, but scrolling through the emails sounds cool! We should keep the icon gray since our design system currently doesn't include colors for icon buttons.

@ehconitin
Copy link
Contributor

video reupload -
@Bonapara

371299428-7a4f6f9a-7fef-42f1-a226-59a1d73767f4.mov

@Bonapara
Copy link
Member

Let's make the trash icons gray and we're good!

@harshit078
Copy link
Contributor Author

@Bonapara Fixed the icon !

@ehconitin
Copy link
Contributor

1

@harshit078 Thanks for this PR, could you also handle these changes?

  1. Align the Email column header and its corresponding content to the left.
  2. Align the Expires In column header to the left.
  3. Ensure that the email icon stays fixed in place while the email text scrolls if necessary.

@harshit078
Copy link
Contributor Author

1

@harshit078 Thanks for this PR, could you also handle these changes?

  1. Align the Email column header and its corresponding content to the left.
  2. Align the Expires In column header to the left.
  3. Ensure that the email icon stays fixed in place while the email text scrolls if necessary.

Note

  • Fixed email column header to left
  • The expires in column is aligned with its content when refreshed then only the tag takes width
  • Gave smooth and fixed scroll animation which makes it scroll back to its position and only gave scroll to mobile viewport
Screen.Recording.2024-09-29.at.4.20.04.PM.mov

@ehconitin
Copy link
Contributor

@Bonapara

2024-09-30.19-09-19.mp4

@Bonapara
Copy link
Member

Looks 1) is not fixed yet, right @harshit078?
We can keep 2) aligned to the right; it's the desired behavior.
3) couldn't view in video, are icons scrolling to the left? It's a good idea to keep them sticky indeed!

Thanks to you both ;)

@harshit078
Copy link
Contributor Author

@Bonapara, I fixed all the concerns that you mentioned.

  • Fixed email icon and emails scroll behind it for a smooth UI
  • Fixed (1)
Screen.Recording.2024-10-01.at.1.26.11.AM.mov

@ehconitin
Copy link
Contributor

Before:
image

After:
image

Before:
image

After:
image

(1) seems to be still broken, its not just headers left alignment but also its corresponding cells :)
for now it seems to be broken on desktop width also

@Bonapara
Copy link
Member

Bonapara commented Oct 1, 2024

@ehconitin thanks! Can we keep the Text/primary color for the emails & icons? Looks it was changed to "secondary" in the "after" screenshot. Also Looks like the mail icons are no longer properly vertically aligned with the email in the desktop "after" screen.

@harshit078
Copy link
Contributor Author

@ehconitin, I've made the changes.

  • Align icons under TableHeader
  • Fixed( 1)

Could you check for on your localhost (1) ?

@ehconitin
Copy link
Contributor

Untitled design (2)

@harshit078
in the before attached image in above comments, you see how emails are aligned left?
and your changes make it unaligned.
The emails should be appearing after the red line ie how it appears in before images :)

@harshit078
Copy link
Contributor Author

There are two edge cases with the Members table -

  • If name of the user is long
  • If email of the user is long

To tacke these edge cases -

  • To handle user name long, name has white-space: pre-line; which be ideal scenario
  • To handle long emails, same propety as invite by email table is given which is scroll.
Screen.Recording.2024-10-07.at.2.47.01.AM.mov

When name not long and email is long

Screenshot 2024-10-07 at 2 47 36 AM

General Scenario

Screenshot 2024-10-07 at 2 49 54 AM

@ehconitin
Copy link
Contributor

@charlesBochet could you please take a look?
Thanks

Copy link
Contributor

@martmull martmull left a comment

Choose a reason for hiding this comment

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

LGTM

@martmull martmull merged commit 098551b into twentyhq:main Oct 8, 2024
11 checks passed
Copy link

github-actions bot commented Oct 8, 2024

Thanks @harshit078 for your contribution!
This marks your 24th 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#7253
- Made the invite table mobile friendly for all media width

## Before


https://github.com/user-attachments/assets/458bd47d-38fb-4ddc-a996-c1bb3908d014

<img width="439" alt="Screenshot 2024-09-27 at 1 30 52 AM"
src="https://github.com/user-attachments/assets/2a0ba6a2-c0f6-42bb-b74d-3a3147f2e7e7">

## After

<img width="440" alt="Screenshot 2024-09-27 at 1 34 11 AM"
src="https://github.com/user-attachments/assets/d31fdeba-574a-4cd0-a61a-bb5fba656109">


https://github.com/user-attachments/assets/7a4f6f9a-7fef-42f1-a226-59a1d73767f4


> [!Note]
> I've added 2 implementations and if either doesn't follow design rules
then it can be changed-
> - Made the trash icon `accent danger`
> - When emails are long, given scroll for ease of convience.

---------

Co-authored-by: Nitin Koche <[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.

5 participants