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

fix(tabs): preserve scroll position when switching between tabs #6812

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

fix(tabs): preserve scroll position when switching between tabs #6812

wants to merge 1 commit into from

Conversation

crisbeto
Copy link
Member

@crisbeto crisbeto commented Sep 2, 2017

Preserves the scroll position when switching between tabs. Previously it was being reset to 0, because we detach and re-attach the content.

Fixes #6722.

@googlebot googlebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Sep 2, 2017
@crisbeto crisbeto force-pushed the 6722/tabs-scroll-position branch from 298b02b to 30812f0 Compare September 3, 2017 15:26
andrewseguin
andrewseguin reviewed Sep 7, 2017
View reviewed changes
Copy link
Contributor

@andrewseguin andrewseguin left a comment

Choose a reason for hiding this comment

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

Looks good on Chrome, but Android was giving me a little trouble. On the devapp, I scrolled to the bottom of Tab Three under Tab Group Demo - Fixed Height example, then switched between Tab Four and Tab Three. Each time I switched to Tab Three, the scroll was a bit higher than when I had left it

@andrewseguin andrewseguin assigned crisbeto and unassigned andrewseguin Oct 6, 2017
@jelbourn
Copy link
Member

@crisbeto @andrewseguin should we revisit this or close?

@crisbeto
Copy link
Member Author

I think it's still a valid issue, but I haven't gotten around to finding an Android phone to test the issue that @andrewseguin mentioned.

@jelbourn
Copy link
Member

AFAIK Chrome dev tools should mimic Android Chrome pretty well. Was it not reproducible there?

@crisbeto
Copy link
Member Author

crisbeto commented Jan 26, 2018 •
edited
Loading

AFAIK the Chrome dev tools only resize the viewport, simulate touch events and fake the user agent.

@josephperrott
Copy link
Member

@crisbeto Please rebase when you have a chance.

@crisbeto crisbeto force-pushed the 6722/tabs-scroll-position branch 2 times, most recently from 8d5aefe to c16f8e3 Compare March 31, 2018 08:07
@crisbeto
Copy link
Member Author

Rebased.

@crisbeto crisbeto force-pushed the 6722/tabs-scroll-position branch from c16f8e3 to 6e695aa Compare April 24, 2018 02:19
melroy89
melroy89 previously approved these changes May 2, 2018
View reviewed changes
@josephperrott josephperrott added the target: minor This PR is targeted for the next minor release label Jun 26, 2018
@ngbot
Copy link

ngbot bot commented Jul 11, 2018

Hi @crisbeto! This PR has merge conflicts due to recent upstream merges.
Please help to unblock it by resolving these conflicts. Thanks!

@crisbeto crisbeto force-pushed the 6722/tabs-scroll-position branch from 6e695aa to e67fda6 Compare July 14, 2018 12:19
@crisbeto crisbeto force-pushed the 6722/tabs-scroll-position branch from e67fda6 to afc4f88 Compare July 21, 2018 18:08
@crisbeto crisbeto mentioned this pull request Sep 7, 2018
Open
@andrewseguin
Copy link
Contributor

Hey @crisbeto - if you don't mind rebasing, we can try to get this pushed in

@crisbeto crisbeto force-pushed the 6722/tabs-scroll-position branch 2 times, most recently from 9521d84 to 59ea92d Compare February 16, 2019 08:53
@crisbeto
Copy link
Member Author

It's good to go now @andrewseguin.

andrewseguin
andrewseguin previously approved these changes Feb 19, 2019
View reviewed changes
Copy link
Contributor

@andrewseguin andrewseguin left a comment

Choose a reason for hiding this comment

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

LGTM, you can add merge ready label if you can verify that the function is not used and can be removed

src/lib/tabs/tab-body.ts Outdated
}
}

_onTranslateTabComplete(e: AnimationEvent): void {
Copy link
Contributor

Choose a reason for hiding this comment

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

This is unused

@andrewseguin
Copy link
Contributor

Also, do you mind adding in this example to the Material Examples? We don't currently have a good example of scrolling content in our tabs and it can be used to double-check the functionality

https://stackblitz.com/edit/angular-vtgxfn?file=main.ts

@crisbeto
fix(tabs): preserve scroll position when switching between tabs
26dd88c 
Preserves the scroll position when switching between tabs. Previously it was being reset to 0, because we detach and re-attach the content.

Fixes angular#6722.
@crisbeto crisbeto force-pushed the 6722/tabs-scroll-position branch from 59ea92d to 26dd88c Compare February 19, 2019 18:55
@crisbeto crisbeto requested a review from jelbourn as a code owner February 19, 2019 18:55
@crisbeto
Copy link
Member Author

Done @andrewseguin.

jelbourn
jelbourn reviewed Feb 19, 2019
View reviewed changes
src/lib/tabs/tab-body.ts
position == 'right-origin-center';
}

_restoreScrollPosition() {
Copy link
Member

Choose a reason for hiding this comment

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

I could see this being on option on the tabs rather than always doing it. @andrewseguin thoughts?

@deftomat
Copy link

Any news?

@mmalerba mmalerba added aaa and removed aaa labels Apr 25, 2019
@andrewseguin andrewseguin added the P4 A relatively minor issue that is not relevant to core functions label May 30, 2019
@jelbourn
Copy link
Member

(revisiting old PRs)
Keeping this one open since we still want to land it, but still lower priority than other issues.

@andrewseguin andrewseguin removed the cla: yes PR author has agreed to Google's Contributor License Agreement label Dec 29, 2021
@googlebot googlebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Dec 29, 2021
@andrewseguin andrewseguin removed the P4 A relatively minor issue that is not relevant to core functions label Mar 28, 2022
@andrewseguin andrewseguin dismissed stale reviews from melroy89 and themself via 26dd88c May 2, 2022 20:33
@josephperrott josephperrott added action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews and removed needs rebase labels Nov 16, 2022
@josephperrott josephperrott requested review from a team as code owners December 18, 2024 17:40
@josephperrott josephperrott requested review from amysorto and mmalerba and removed request for a team December 18, 2024 17:40
@mmalerba mmalerba removed their request for review February 20, 2025 00:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jelbourn jelbourn jelbourn left review comments

@melroy89 melroy89 melroy89 left review comments

@andrewseguin andrewseguin andrewseguin left review comments

@amysorto amysorto Awaiting requested review from amysorto amysorto is a code owner automatically assigned from angular/components-googlers

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

Assignees
No one assigned
Labels
action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews cla: yes PR author has agreed to Google's Contributor License Agreement target: minor This PR is targeted for the next minor release
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

The scroll bar inside a md-tab can not retain its position when switching between tabs
8 participants
@crisbeto @jelbourn @josephperrott @andrewseguin @deftomat @melroy89 @googlebot @mmalerba