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

perf(tabs): use a transform to resize and move the ink bar #10664

Merged
merged 1 commit into from
May 29, 2018

Conversation

crisbeto
Copy link
Member

@crisbeto crisbeto commented Apr 2, 2018

Uses a transform, rather than left and width to move/resize the ink bar in order to ensure a smoother transition.

@crisbeto crisbeto requested a review from andrewseguin as a code owner April 2, 2018 11:22
@googlebot googlebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Apr 2, 2018

/**
* Interface for a a MatInkBar positioner method, defining the positioning and width of the ink
* bar in a set of tabs.
* @deprecated Signature to be changed to `{left: number, width: number}`.
Copy link
Member

Choose a reason for hiding this comment

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

Since this is internal, we can just change the signature now. No need to go through the @deprecated path

Copy link
Member Author

@crisbeto crisbeto Apr 2, 2018

Choose a reason for hiding this comment

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

That's fine by me, but I thought the intention with #9972 was to make this logic public.

Copy link
Member

Choose a reason for hiding this comment

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

At this point its still internal to the library, though we might make it public later. But since it's still internal at this point, we can just switch it over.

Copy link
Member Author

Choose a reason for hiding this comment

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

Cool, done.

@crisbeto crisbeto force-pushed the ink-bar-transform branch from 7d65167 to d141faf Compare April 2, 2018 16:03
@josephperrott josephperrott added pr: lgtm action: merge The PR is ready for merge by the caretaker target: patch This PR is targeted for the next patch release and removed pr: needs review labels Apr 2, 2018
@crisbeto crisbeto force-pushed the ink-bar-transform branch from d141faf to 7e25889 Compare April 5, 2018 21:16
@tinayuangao tinayuangao added the presubmit failures This PR has failures in Google's internal presubmit process and cannot be immediately merged label Apr 12, 2018
@tinayuangao
Copy link
Contributor

The current tab indicator (colored line under the tab) disappeared in screenshot tests.

@andrewseguin
Copy link
Contributor

When the tabs are first rendered, the ink bar transitions into place. I believe this is a regression

Old behavior: https://stackblitz.com/edit/angular-32czww?file=app/tabs-overview-example.ts

The ink bar is immediately present on first render and only translates when changed.

With this PR, open the demo app and load the tabs page. All the ink bars transition into place on first render.

This is the reason why we are seeing screenshot errors I believe, because the ink bar is 1px wide as if its about to render.

@andrewseguin andrewseguin removed the action: merge The PR is ready for merge by the caretaker label Apr 26, 2018
@crisbeto crisbeto force-pushed the ink-bar-transform branch from 7e25889 to 9829b80 Compare May 3, 2018 21:32
Uses a transform, rather than `left` and `width` to move/resize the ink bar in order to ensure a smoother transition.
@crisbeto crisbeto force-pushed the ink-bar-transform branch from 9829b80 to 63b6fd9 Compare May 27, 2018 08:06
@crisbeto
Copy link
Member Author

@andrewseguin @tinayuangao I've reworked it to prevent it from animating in on the initial load. Marking as merge ready to see if it still breaks anything.

@crisbeto crisbeto added action: merge The PR is ready for merge by the caretaker and removed presubmit failures This PR has failures in Google's internal presubmit process and cannot be immediately merged labels May 27, 2018
@josephperrott josephperrott merged commit b090f6d into angular:master May 29, 2018
@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 Sep 9, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
action: merge The PR is ready for merge by the caretaker cla: yes PR author has agreed to Google's Contributor License Agreement target: patch This PR is targeted for the next patch release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants