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

Repaint fixes #13410

Closed
wants to merge 12 commits into from
Closed

Repaint fixes #13410

wants to merge 12 commits into from

Conversation

ItsJonQ
Copy link
Contributor

@ItsJonQ ItsJonQ commented Apr 23, 2014

Hey hey,
Applied translateZ / translate3d to fixed elements to prevent browser repaint when scrolling.

These changes were originally part of a Bootstrap reset I made called "Lil' B"
http://itsjonq.github.io/lil-b/

Also added a new mixin under vendor-prefixes.less - .translateZ(@z). This targets translateZ directly.
I initially wanted to use .translate3d to target the "Z" axis for the repaint fixes.. but I believe translateZ has IE9 whereas translate3d doesn't:

http://caniuse.com/transforms3d

Hope this helps!

For you guys:
https://pbs.twimg.com/media/Bl3XWPkIUAAm66k.jpg:large

@cvrebert cvrebert modified the milestone: v3.2.0 Apr 23, 2014
@cvrebert
Copy link
Collaborator

Not sure whether we should really use a mixin here. We've switched to Autoprefixer for handling vendor prefixing, which made all of our vendor prefix mixins deprecated (& subject to deletion in v4), so it seems weird to add a new one that'll be deprecated right from the get-go.

@NickColley
Copy link
Contributor

will-change might be be of relevance to this http://www.aerotwist.com/blog/bye-bye-layer-hacks/
Putting it here incase it helps. 👍

@ItsJonQ
Copy link
Contributor Author

ItsJonQ commented Apr 23, 2014

@cvrebert Oh, I didn't know about that change in v4. If you're down for it, I can make the repaint-fix changes without the new mixin if you think that'll help for v3.

@NickColley That's awesome! I've only started following Paul's stuff recently - must have missed that one. Very excited to see the future of will-change.

@cvrebert
Copy link
Collaborator

@ItsJonQ It's @mdo's call; just wanted to make sure to raise the issue.

@ItsJonQ
Copy link
Contributor Author

ItsJonQ commented Apr 24, 2014

Heya,

Just made an update and push that removed the .translateZ mixin, and added .translate3d in it's place for the navbars and .affix.

@NickColley
Copy link
Contributor

As cvrebert mentioned Bootstrap uses autoprefixer so you could just have the pure transform property, yay less code.

Might be It'd be nice to do something like:

.will-change {
  transform: translate3d(0, 0, 0);
}
.affix {
  .will-change();
}

Frees you up to changing the implementation incase it changes in the future.

@XhmikosR
Copy link
Member

Also this needs to be rebased against the latest master @ItsJonQ.

@cvrebert
Copy link
Collaborator

@XhmikosR It's probably just the *.min.* & *.map files. I'm happy to do the rebase once @mdo finishes reviewing this.

@XhmikosR
Copy link
Member

OK, cool. It's also the rtl files that have been removed.

@ItsJonQ
Copy link
Contributor Author

ItsJonQ commented May 13, 2014

Thanks for the heads up! The changes were made a while ago, and I haven't updated anything as I was waiting to hear back.

I can make whatever updates/changes needed - just lemme know :)

bump grunt-saucelabs to v5.1.3
@XhmikosR
Copy link
Member

@ItsJonQ: you will need to rebase against the latest upstream master branch.

@NickColley
Copy link
Contributor

@ItsJonQ also you'll need to remove the vendor prefixes too as I mentioned above 👍

@ItsJonQ
Copy link
Contributor Author

ItsJonQ commented May 14, 2014

Hey guys,
I'm not super familiar with rebasing (sorry!). I followed the instructions from CONTRIBUTING.md the best that I could. It took me about an hour of tinkering, but I was able to push.

I hope everything went through okay. If you guys have some free time, I'd really appreciate it if you guys can double check to make sure that I didn't break anything.

Thanks!

@XhmikosR
Copy link
Member

Nah, it's still wrong @ItsJonQ. You shouldn't have changes in non existent files like the RTL files or the merge patches.

https://help.github.com/articles/interactive-rebase

So basically git rebase -i upstream/master assuming you have the upstream repo like that.

@mdo
Copy link
Member

mdo commented May 14, 2014

Alternatively, if you're like me @ItsJonQ, you can open a new PR :). Let's figure out any final feedback here first though. I'll dive in...

@XhmikosR
Copy link
Member

Or someone from us can cherry pick the changes if needed, as long as they are reviewed :)

@mdo
Copy link
Member

mdo commented May 14, 2014

As far as I can tell, this all looks great, aside from the unrelated files @XhmikosR mentioned and the un-prefixed, non-mixin version of transform: translate3d(0, 0, 0); mentioned by @NickColley.

Make those changes and open a new PR (preferably with a single commit), or try the rebase and squash thing again if you like. Your call :).

@ItsJonQ
Copy link
Contributor Author

ItsJonQ commented May 14, 2014

@mdo @XhmikosR @NickColley

Thanks guys for being so patient + understanding :). This is the first time I'm contributing to a public project - I've learned a ton so far!

@mdo I think it would be easiest to open a new pull request, haha.

Thanks again. You guys rock \m/

@mdo
Copy link
Member

mdo commented May 14, 2014

@ItsJonQ Awesome! Go ahead and close this and open a new one when you're all set. Thanks so much—and for it being your first, you're doing great <3.

@cvrebert
Copy link
Collaborator

Successor: #13649.

@cvrebert cvrebert closed this May 21, 2014
@twbs twbs locked and limited conversation to collaborators Jun 23, 2015
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants