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(material/tabs): stop scrolling on tab change #22265

Merged
merged 3 commits into from
Apr 27, 2021
Merged

fix(material/tabs): stop scrolling on tab change #22265

merged 3 commits into from
Apr 27, 2021

Conversation

ukrukarg
Copy link
Contributor

Adds min-height to the mat-tab-group wrapper, so page height is preserved
as tabs change, and page doesn't scroll up.

Fixes #9592

@ukrukarg ukrukarg requested a review from andrewseguin as a code owner March 17, 2021 21:06
@google-cla google-cla bot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Mar 17, 2021
@ukrukarg ukrukarg requested a review from crisbeto as a code owner March 17, 2021 22:04
crisbeto
crisbeto reviewed Mar 18, 2021
View reviewed changes
Copy link
Member

@crisbeto crisbeto left a comment

Choose a reason for hiding this comment

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

Is there an example of the problem that this fixes? I tried applying the changes to our dev app, but I couldn't notice much of a difference. Furthermore, I noticed that the active tab body had a min-height: 1px so the measurement might be happening at the wrong time.

Also cc @andrewseguin

@ukrukarg
Copy link
Contributor Author

https://stackblitz.com/edit/mat-tabs-scroll-page-top-on-tab-change?file=app/tabs-template-label-example.html showcases the problem. Steps to repro:

  • scroll page a little
  • click on tab header
  • notice page scrolled to the top

@ukrukarg ukrukarg requested a review from crisbeto March 22, 2021 15:01
crisbeto
crisbeto approved these changes Mar 22, 2021
View reviewed changes
Copy link
Member

@crisbeto crisbeto left a comment

Choose a reason for hiding this comment

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

LGTM with a couple of non-blocking nits.

src/material/tabs/tab-group.spec.ts Outdated Show resolved Hide resolved
src/material/tabs/tab-group.ts Show resolved Hide resolved
@crisbeto crisbeto added action: merge The PR is ready for merge by the caretaker P3 An issue that is relevant to core functions, but does not impede progress. Important, but not urgent target: patch This PR is targeted for the next patch release labels Mar 22, 2021
@andrewseguin andrewseguin removed the action: merge The PR is ready for merge by the caretaker label Mar 23, 2021
@andrewseguin
Copy link
Contributor

@ukrukarg Tests are passing both externally and internally so we should be good to merge this. Do you mind addressing the latest comments?

@ukrukarg
Copy link
Contributor Author

Done.

@ukrukarg
Copy link
Contributor Author

ukrukarg commented Apr 6, 2021

Hmm, tests failed with some kind of environment problem it looks like (not related to my change), I cannot rerun them, can somebody else rerun?

@crisbeto
Copy link
Member

Sorry for the delay response @ukrukarg, I must have missed the notification. Can you rebase the PR against the latest master to make sure the CI passes?

ukrukarg added 3 commits April 23, 2021 12:54
@ukrukarg
fix(material/tabs): stop scrolling on tab change
173458a 
Adds min-height to the mat-tab-group wrapper, so page height is preserved
as tabs change, and page doesn't scroll up.

Fixes #9592
@ukrukarg
fix(material/tabs): stop scrolling on tab change
0dc0add 
Adds min-height to the mat-tab-group wrapper, so page height is preserved
as tabs change, and page doesn't scroll up.

Fixes #9592
@ukrukarg
fix(material/tabs): stop scrolling on tab change …
e624b9e 
Adds min-height to the mat-tab-group wrapper, so page height is preserved
as tabs change, and page doesn't scroll up.

Fixes #9592
@crisbeto crisbeto added the action: merge The PR is ready for merge by the caretaker label Apr 23, 2021
@ukrukarg
Copy link
Contributor Author

Done.

@andrewseguin andrewseguin merged commit c12f168 into angular:master Apr 27, 2021
andrewseguin pushed a commit that referenced this pull request Apr 27, 2021
@ukrukarg @andrewseguin
fix(material/tabs): stop scrolling on tab change (#22265)
7f556b5 
* fix(material/tabs): stop scrolling on tab change

Adds min-height to the mat-tab-group wrapper, so page height is preserved
as tabs change, and page doesn't scroll up.

Fixes #9592

* fix(material/tabs): stop scrolling on tab change

Adds min-height to the mat-tab-group wrapper, so page height is preserved
as tabs change, and page doesn't scroll up.

Fixes #9592

* fix(material/tabs): stop scrolling on tab change …

Adds min-height to the mat-tab-group wrapper, so page height is preserved
as tabs change, and page doesn't scroll up.

Fixes #9592

(cherry picked from commit c12f168)
andrewseguin pushed a commit that referenced this pull request Apr 27, 2021
@ukrukarg @andrewseguin
fix(material/tabs): stop scrolling on tab change (#22265)
5ecefa7 
* fix(material/tabs): stop scrolling on tab change

Adds min-height to the mat-tab-group wrapper, so page height is preserved
as tabs change, and page doesn't scroll up.

Fixes #9592

* fix(material/tabs): stop scrolling on tab change

Adds min-height to the mat-tab-group wrapper, so page height is preserved
as tabs change, and page doesn't scroll up.

Fixes #9592

* fix(material/tabs): stop scrolling on tab change …

Adds min-height to the mat-tab-group wrapper, so page height is preserved
as tabs change, and page doesn't scroll up.

Fixes #9592

(cherry picked from commit c12f168)
@angular-automatic-lock-bot
Copy link

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators May 28, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@crisbeto crisbeto crisbeto approved these changes

@andrewseguin andrewseguin Awaiting requested review from andrewseguin

Assignees
No one assigned
Labels
action: merge The PR is ready for merge by the caretaker cla: yes PR author has agreed to Google's Contributor License Agreement P3 An issue that is relevant to core functions, but does not impede progress. Important, but not urgent target: patch This PR is targeted for the next patch release
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Tabs] Automatically page scrolls to top when switching between some tabs
3 participants
@ukrukarg @andrewseguin @crisbeto