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: Restrict Timeline Feed scrolling to its column on /me route #10649

Conversation

iamsmruti
Copy link
Contributor

Description

This PR fixes the issue where scrolling the Timeline Feed on the /me route would cause the "Active Tasks" column, to scroll as well. The Timeline Feed column now scrolls independently, ensuring the "Active Tasks" column remains static.

Fixes #10648
I had to remove the overflow: 'auto' from the FeedAndDrawer and add it to just the TimelineFeed

Demo

after-change.mov

Testing scenarios

  • Scenario A: Ensure the Timeline Feed scrolls independently.
    • Step 1: Navigate to the /me route.
    • Step 2: Populate the Timeline Feed with enough content to cause vertical overflow.
    • Step 3: Scroll within the Timeline Feed column and verify only that column is affected.

Final checklist

  • I checked the code review guidelines
  • I have added Metrics Representative as reviewer(s) if my PR invovles metrics/data/analytics related changes
  • I have performed a self-review of my code, the same way I'd do it for any other team member
  • I have tested all cases I listed in the testing scenarios and I haven't found any issues or regressions
  • Whenever I took a non-obvious choice I added a comment explaining why I did it this way
  • I added the label Skip Maintainer Review Indicating the PR only requires reviewer review and can be merged right after it's approved if the PR introduces only minor changes, does not contain any architectural changes or does not introduce any new patterns and I think one review is sufficient'
  • PR title is human readable and could be used in changelog

@github-actions github-actions bot requested a review from Dschoordsch January 8, 2025 18:18
@iamsmruti iamsmruti changed the title Fix: Restrict Timeline Feed scrolling to its column on /me route fix: Restrict Timeline Feed scrolling to its column on /me route Jan 8, 2025
Copy link
Contributor

@Dschoordsch Dschoordsch left a comment

Choose a reason for hiding this comment

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

Works nice for the timeline, but does break scrolling for active tasks if there are a lot of them, see loom

@iamsmruti
Copy link
Contributor Author

Works nice for the timeline, but does break scrolling for active tasks if there are a lot of them, see loom

Alright, I am making the changes for that as well.

@iamsmruti iamsmruti force-pushed the fix/timeline-scroll-affects-active-tasks branch from ea3d2d3 to a3f0aa8 Compare January 9, 2025 14:17
@iamsmruti iamsmruti requested a review from Dschoordsch January 9, 2025 14:22
@iamsmruti
Copy link
Contributor Author

@Dschoordsch I have fixed the Active Tasks as well. Please have a look

Screen.Recording.2025-01-09.at.7.57.46.PM.mov

Copy link
Contributor

@Dschoordsch Dschoordsch left a comment

Choose a reason for hiding this comment

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

Thanks for the update. It works fine, but I don't like how it feels. Having two scrollbars feels too cluttered and I think we don't need the two separate scrolling contexts as I don't see a use case where a user needs to see a specific task and history event at the same time.
So what I think would be better is going back to the state of master and instead fix the "Active Tasks" column to run the full length, so that the border shows correctly and scrolling works regardless if there are more tasks or more history events.

@iamsmruti
Copy link
Contributor Author

iamsmruti commented Jan 10, 2025

Thanks for the update. It works fine, but I don't like how it feels. Having two scrollbars feels too cluttered and I think we don't need the two separate scrolling contexts as I don't see a use case where a user needs to see a specific task and history event at the same time. So what I think would be better is going back to the state of master and instead fix the "Active Tasks" column to run the full length, so that the border shows correctly and scrolling works regardless if there are more tasks or more history events.

Well, if you say, it's not important to see a specific event and history at the same time, then your proposition is correct. But I feel both things are separate entities, and should have their scrollable area. We have scrollable areas in tasks as well. Right ?

I will surely do a POC about your proposition and see how things roll out. Will let you know about this.

Let me know your thoughts,

@Dschoordsch
Copy link
Contributor

I agree these are 2 different things, but I don't think there is a use case where I want to see past event E together with a specific task T. Scrolling in a task is also not that nice and we should probably fix it to expand the task instead. If you feel strongly that we should scroll both independently, I would be interested in your use case.

@iamsmruti
Copy link
Contributor Author

I got your point. Let's do this. I am updating the PR description as well.

@iamsmruti iamsmruti force-pushed the fix/timeline-scroll-affects-active-tasks branch from a3f0aa8 to c9c5095 Compare January 10, 2025 18:24
@iamsmruti
Copy link
Contributor Author

I think I like this layout as well. Have a look @Dschoordsch

Screen.Recording.2025-01-10.at.11.55.01.PM.mov

Copy link
Contributor

@Dschoordsch Dschoordsch left a comment

Choose a reason for hiding this comment

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

Thanks for your work! Yes, this looks much nicer without the scrollbar in the middle of the page.
Will merge it as soon as the tests pass.

@Dschoordsch Dschoordsch merged commit 052647f into ParabolInc:master Jan 13, 2025
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bug: Scrolling Timeline Feed Affects Active Tasks Column on /me Route
2 participants