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

feat: improved table scrolling #7731

Merged

Conversation

Khaan25
Copy link
Contributor

@Khaan25 Khaan25 commented Oct 16, 2024

This PR fixes #7515

Demo:

2024-10-16.09-06-19.mp4

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 PR enhances table scrolling by implementing smoother transitions and adjusting column widths, addressing the issues raised in #7515, particularly for mobile devices.

  • Added transition effect (0.3s ease) to header cells in RecordTableHeaderCell.tsx for smoother scrolling
  • Increased left positioning of second and third columns in RecordTableBodyDroppable.tsx for better alignment
  • Removed 'hideTitle' prop from StyledTitle in RecordTableColumnHead.tsx, potentially affecting mobile view
  • Significantly increased third column width (30px to 150px) for mobile devices in RecordTableHeader.tsx, which may impact small screen layouts

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

@@ -30,6 +30,7 @@ const StyledColumnHeaderCell = styled.th<{
color: ${({ theme }) => theme.font.color.tertiary};
padding: 0;
text-align: left;
transition: 0.3s ease;
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 shorter duration for a snappier feel, e.g., '0.2s ease'

@Bonapara
Copy link
Member

Hi @Khaan25 we want to keep the following behavior on mobile:
image

@Khaan25
Copy link
Contributor Author

Khaan25 commented Oct 16, 2024

Alright. Will do it

@Khaan25
Copy link
Contributor Author

Khaan25 commented Oct 16, 2024

2024-10-16.19-37-06.mp4

@Bonapara 🤝

@Khaan25
Copy link
Contributor Author

Khaan25 commented Oct 16, 2024

@Bonapara Can you please close this? I can see another issue open, I cna work on that?

@Bonapara
Copy link
Member

Bonapara commented Oct 16, 2024

@Khaan25, this looks great! Can you make these two fixes?

Desired:

The shape of mobile chips should be square:

image

Current:

the chip has an extra right padding

CleanShot 2024-10-16 at 17 47 59


Also can you add a small margin at the right of the chips & + button?

Desired

CleanShot 2024-10-16 at 17 49 15

Current

(No margin right)

CleanShot 2024-10-16 at 17 50 11

@Khaan25
Copy link
Contributor Author

Khaan25 commented Oct 16, 2024

Sure. I was thinking to ask you about this - Great you brought it up 💪

@Khaan25
Copy link
Contributor Author

Khaan25 commented Oct 16, 2024

One more question, Can I unassign myself from the Issue and assign myself to any other issue?

Will I get points for this PR as well or first finish it then move on?

@Bonapara
Copy link
Member

We should finish so we don't lose track of things. Additionally, the bot doesn't allow assigning oss.gg issues to someone if they are already assigned to another issue..

@Khaan25
Copy link
Contributor Author

Khaan25 commented Oct 16, 2024

RIght. It was a simple CSS fix so I thought I could get points for that one as well while you review this one :)

Anyway, Thank you. It's my first time contributing open source

@Bonapara
Copy link
Member

If it's a minor CSS fix, let's make an exception. Tag me, and I'll assign it to you if no one else is assigned ;)

@Khaan25
Copy link
Contributor Author

Khaan25 commented Oct 16, 2024

But now it's too late, someone assign himself 😄 - I'll tag you next time. Thank you for the support

@Khaan25
Copy link
Contributor Author

Khaan25 commented Oct 16, 2024

Can you assign this to me? #7757

@Khaan25 Khaan25 closed this Oct 16, 2024
@Khaan25 Khaan25 reopened this Oct 16, 2024
@Khaan25
Copy link
Contributor Author

Khaan25 commented Oct 16, 2024

Can you assign this to me? #7757

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

(updates since last review)

This PR continues to refine table scrolling improvements, focusing on transition effects and mobile responsiveness.

  • Added transition effect (0.3s ease) to table body cells in RecordTableBodyDroppable.tsx for consistent smooth scrolling
  • Implemented 'hideTitle' prop in RecordTableColumnHead.tsx to improve first column readability on mobile
  • Fine-tuned positioning and sizing of header elements in RecordTableHeader.tsx for better mobile scrolling experience
  • Ensured consistent transition effects across table components for a unified scrolling behavior

4 file(s) reviewed, 4 comment(s)
Edit PR Review Bot Settings | Greptile

z-index: 5;
transition: 0.3s ease;
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 applying transition to all sticky columns for consistent behavior

@Bonapara
Copy link
Member

Sorry, I wasn't quick enough, which is why it's not that simple 😅

@Khaan25
Copy link
Contributor Author

Khaan25 commented Oct 16, 2024

You can unassign him and assign me? 😂

P.S. wrapping up this one as well

@Bonapara
Copy link
Member

That would not be fair :)

@Khaan25
Copy link
Contributor Author

Khaan25 commented Oct 16, 2024

Haha, I know but if it occurs again?

Can I unassign myself from this issue and assign myself to new issue and still get points for PR? If that's the option then it would be great :)

@Khaan25
Copy link
Contributor Author

Khaan25 commented Oct 16, 2024

Quick question, if you don't mind, How do I see the issues that have points and are not assigned? Can't figure that out from the search :(

Have to search from the long list.

@Bonapara
Copy link
Member

@Khaan25
Copy link
Contributor Author

Khaan25 commented Oct 16, 2024

Can you please assign this to me? #4588

@Bonapara
Copy link
Member

You need to comment on it

@Khaan25
Copy link
Contributor Author

Khaan25 commented Oct 18, 2024

Perfect. Gimme few minutes

@Khaan25
Copy link
Contributor Author

Khaan25 commented Oct 18, 2024

@Bonapara Fixed

@Khaan25
Copy link
Contributor Author

Khaan25 commented Oct 19, 2024

Hey, could you review it? I can work on other issues?

@Bonapara
Copy link
Member

@lucasbordeau looks good to me from a design point of view if you want to proceed with the code review

@Khaan25
Copy link
Contributor Author

Khaan25 commented Oct 23, 2024

Hey hey, any update on this? :)

@Khaan25
Copy link
Contributor Author

Khaan25 commented Oct 25, 2024

Any update :)

@Bonapara
Copy link
Member

@lucasbordeau will review it today @Khaan25 ;)

@lucasbordeau lucasbordeau enabled auto-merge (squash) October 25, 2024 08:41
@Khaan25
Copy link
Contributor Author

Khaan25 commented Oct 25, 2024

@Bonapara Finally it's done 🎉:)

@Khaan25
Copy link
Contributor Author

Khaan25 commented Oct 27, 2024

@Bonapara, could you please assign me points for this one? :)

@Bonapara
Copy link
Member

/award 300

Copy link

oss-gg bot commented Oct 28, 2024

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

@Devessier
Copy link
Contributor

Devessier commented Oct 28, 2024

I'm unsure if we want to rely on the data-testid attributes for styling. It should be used only in e2e tests.

@media (max-width: ${MOBILE_VIEWPORT}px) {
& [data-testid='editable-cell-display-mode'] {
[data-testid='tooltip'] {
display: none;
}
[data-testid='chip'] {
gap: 0;
}
}
}

@lucasbordeau
Copy link
Contributor

@Bonapara Is it ok for you ?

@Bonapara
Copy link
Member

Bonapara commented Nov 4, 2024

Yes @lucasbordeau, I think it was!

@lucasbordeau
Copy link
Contributor

Follow-up issue : #8307

@lucasbordeau lucasbordeau merged commit 66c0aa5 into twentyhq:main Nov 4, 2024
17 checks passed
Copy link

github-actions bot commented Nov 4, 2024

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

Contributions

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.

Improve scrolling in the table
5 participants