Skip to content
This repository has been archived by the owner on Aug 30, 2018. It is now read-only.

Prefix mixins cleanup #410

Merged
merged 9 commits into from
May 25, 2015
Merged

Prefix mixins cleanup #410

merged 9 commits into from
May 25, 2015

Conversation

benknight
Copy link
Contributor

  • Removes unnecessary prefixing for transition, linear-gradient, transform, and box-sizing
  • Replaces prefix mixin with prefixer inspired by Bourbon's prefixer. Encourages only using necessary prefixes.
  • Docs will have to be updated.

Test store: http://timber-dev.myshopify.com/

@cshold
Copy link
Contributor

cshold commented May 12, 2015

This certainly gives you more control over what prefixes will be used, but puts the onus on the dev to know which to supply each time the mixin is used. While they're not all necessary, I'd argue a blanket prefix function eases the dev and reduces chances that any required are missed.

@stevebosworth @mpiotrowicz what do you think?

@stevebosworth
Copy link
Contributor

I agree with @cshold. I think this would ultimately create more problems and complexity since the onus is on the developer to know which prefixes are required which is something that can change as browsers are updated.

If the unnecessary prefixes are causing problems, I would recommend using a prefixer as part of a build process.

@benknight
Copy link
Contributor Author

Having a "prefix-all-the-things" mixin is simple and straightforward solution but not particularly elegant nowadays since the need to prefix anything is quickly disappearing. Almost nothing requires -moz- or -o- (user-select is the only one that comes to mind and that's because it's non-standard).

Using the wrapper pattern like we're doing here for transform will prevent devs from having to memorize which prefixes are needed for commonly-used CSS features. For less commonly-used features, think of the prefixer as a low-level convenience mixin. These are the cases where a dev should be taking an extra 10 seconds to check http://caniuse.com and see which prefixes are needed instead of just carpet bombing the stylesheet with all of them, especially since these can be features that are non-standard and have varying behavior across browsers.

@cshold
Copy link
Contributor

cshold commented May 13, 2015

You make a good point. How about we go with your suggestions, and add back in the backface mixin. On animation heavy sites it's great to just use @include backface() and have it do the magic for you (with hidden as the default.

@benknight
Copy link
Contributor Author

Assuming backface-visibility is being used to fix screen flicker during animations, I suspect it may no longer be necessary. Still though, I've added the backface mixin back in the interest of getting this pull request merged. In a separate pull request I'll submit some suggestions for runtime rendering performance improvements, including using will-change to promote layers to keep the drawer transitions under 60 fps.

@benknight
Copy link
Contributor Author

Hey @cshold, is there anything more you'd like me to do to make this shippable? Let me know 👍

@cshold
Copy link
Contributor

cshold commented May 19, 2015

This looks great. Last thing I'd ask is for a quick link to the boubon source or docs in the comment. Then let's 🚢

@benknight
Copy link
Contributor Author

Alright, I also updated the comment block to match the other ones. I think we're good to go!

@cshold
Copy link
Contributor

cshold commented May 25, 2015

Looks great. I'll add this to the next release and update the docs accordingly. Cheers

cshold added a commit that referenced this pull request May 25, 2015
@cshold cshold merged commit 3ab33cb into Shopify:master May 25, 2015
@cshold cshold mentioned this pull request Jun 9, 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.

3 participants