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

fix(tabs): content animation in RTL not working (chrome) #12215

Merged
merged 1 commit into from
Jul 17, 2018
Merged

fix(tabs): content animation in RTL not working (chrome) #12215

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 8 additions & 2 deletions src/lib/tabs/tabs-animations.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,8 +22,14 @@ export const matTabsAnimations: {
translateTab: trigger('translateTab', [
// Note: transitions to `none` instead of 0, because some browsers might blur the content.
state('center, void, left-origin-center, right-origin-center', style({transform: 'none'})),
state('left', style({transform: 'translate3d(-100%, 0, 0)'})),
state('right', style({transform: 'translate3d(100%, 0, 0)'})),

// If the tab is either on the left or right, we additionally add a `min-height` of 1px
// in order to ensure that the element has a height before its state changes. This is
// necessary because Chrome does seem to skip the transition in RTL mode if the element does
// not have a static height and is not rendered. See related issue: #9465
state('left', style({transform: 'translate3d(-100%, 0, 0)', minHeight: '1px'})),
Copy link
Member

Choose a reason for hiding this comment

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

Can we do this directly in the stylesheet? I think we have a class for the off-screen tabs which should allow us to set the min-height.

Copy link
Member Author

Choose a reason for hiding this comment

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

We only have a class for the active tab body, so we could work with :not, but since this technically belongs to the animation setup I'd anyway prefer having this here.

That way if someone looks for the animations, he can quickly see that there is some special workaround applied.

Copy link
Member

Choose a reason for hiding this comment

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

True, but this makes it a lot more difficult for somebody to change the min-height, if they wanted to.

Copy link
Member Author

@devversion devversion Jul 15, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

That min-height is only temporarily while the tab is hidden. As soon as it transitions to the visible state, the min-height is completely gone.

Also, why would someone need to update the temporary min-height of an internal and hidden element? The developer can still set his own min-height in the projected content easily.

I'm fine changing if you really want to. Just think if we move into some SCSS file, we'd need some special selector (e.g. :not), and at some point we would not remember about that specific workaround that belongs to the animation.

Copy link
Member

Choose a reason for hiding this comment

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

The way I'm viewing it is that it's unlikely for somebody to want to change the min-height, but if they do, it would be very hard for them to do so, whereas it's very low-effort for us to have it in the stylesheet to begin with. Having it there also ends up being less generated code.

Copy link
Member Author

Choose a reason for hiding this comment

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

I see your point with the low-effort to place in the stylesheet. In regards to the code size, I think the generated CSS will take up more space than how it's right now.

Also, if we consider it like that, we could also argue about moving the actual transform's that don't include any computed values to the stylesheet.

Anyway, I will switch it to the CSS but keep a comment that mentions the Chrome workaround that is placed in the stylesheet.

Copy link
Member Author

Choose a reason for hiding this comment

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

As discussed on Slack; since depending on the .mat-tab-body-active does not work because the class is being added before hidden tabs start the actual animation, we are keeping the workaround in the TS file.

state('right', style({transform: 'translate3d(100%, 0, 0)', minHeight: '1px'})),

transition('* => left, * => right, left => center, right => center',
animate('500ms cubic-bezier(0.35, 0, 0.25, 1)')),
transition('void => left-origin-center', [
Expand Down