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

Add preserve option #3

Closed
wants to merge 1 commit into from
Closed

Add preserve option #3

wants to merge 1 commit into from

Conversation

MoOx
Copy link
Contributor

@MoOx MoOx commented May 26, 2014

Setting preserve to true will preserve calc() in the output, so
that they can be used by supporting browsers.
Useful now that browsers are starting to implement vars (eg Firefox
31+) & it’s even better for debug to see real rules.

I’ve refactored the code a little bit to make it easier to use the
preserve in the main function (& also use rework-visit which
is more bullet proof) like rework-vars.

This should be a major update since now calc need to be called as a
function (like rework-vars for example) to be able to pass options. So
this should be tagged as 0.3 (or maybe it’s time to ship 1.0 :) ?)

I also quickly refactor tests to make it more understandable
(plugins.js near preserve.css was confusing imo).

I also add some error reporting like we have in rework-vars. Btw, maybe we can
expand the use of balanced-match to rewrite other parts of the plugin.

I hope it’s ok.

FWI, I did this since I’m more seeing rework-vars & rework-calc as
fallback than replacement.
See reworkcss/rework-vars#28
reworkcss/rework-vars#29 &
segmentio/myth#64

Setting `preserve` to `true` will preserve  `calc()` in the output, so
that they can be used by supporting browsers.
Useful now that browsers are starting to implement vars (eg Firefox
31+) & it’s even better for debug [to see real
rules](http://cl.ly/image/3W3O0E41173X).

I’ve refactored the code a little bit to make it easier to use the
`preserve` in the main function (& also use rework-visit which
is more bullet proof) like rework-vars.

This should be a major update since now calc need to be called as a
function (like rework-vars for example) to be able to pass options. So
this should be tagged as 0.3 (or maybe it’s time to ship 1.0 :) ?)

I also quickly refactor tests to make it more understandable
(plugins.js near preserve.css was confusing imo).

I also add some error reporting like we have in rework-vars. Btw, maybe we can
expand the use of balanced-match to rewrite other parts of the plugin.

I hope it’s ok.

FWI, I did this since I’m more seeing rework-vars & rework-calc as
fallback than replacement.
See reworkcss/rework-vars#28
reworkcss/rework-vars#29 &
segmentio/myth#64
@MoOx
Copy link
Contributor Author

MoOx commented May 28, 2014

Who maintain this repo ? It seems I'm the only one watching it. I can take care of it but not sure to be able to publish something...

poke @joakimbeng @reworkcss/owners

@jonathanong
Copy link
Contributor

take control!

@MoOx
Copy link
Contributor Author

MoOx commented May 28, 2014

@jonathanong I'll need access to npm (be added as a user at least) right (my npm login is moox) ?

@jonathanong
Copy link
Contributor

hmmm i don't have publishing rights either. added you to all the repos i ahve access to

@MoOx
Copy link
Contributor Author

MoOx commented May 28, 2014

Thanks @jonathanong. It seems you are listed here https://www.npmjs.org/package/rework-calc
Anyway I'll wait for a response of @joakimbeng before doing anything.

@jonathanong
Copy link
Contributor

god damn that's not the account i use. fuck you npm!

@necolas
Copy link
Contributor

necolas commented May 28, 2014

There's not really any benefit to preserving calc() in the output, because the browser has to perform exactly the same calculation that can be done ahead of time with the build tool.

@MoOx
Copy link
Contributor Author

MoOx commented May 28, 2014

The idea is to keep custom properties in the dev tools. It's just the same thing as for rework-vars (see screenshot in the PR desc)

@necolas
Copy link
Contributor

necolas commented May 28, 2014

Surely they will be there if you've set rework-vars to preserve, because the use of calc with variables won't be resolved by this module. Plus this changes the API without enough benefit IMO. If you don't need IE8 support, you don't need this module (unless you want to resolve simple calc's ahead of time to avoid the browser needing to).

@MoOx
Copy link
Contributor Author

MoOx commented May 28, 2014

If I use rework-vars as a fallback (so fallback + custom) I want ... Oh wait...

Ok closing this immediatly. I'm so stupid.
I will still send another one to keep my work for error report.

@MoOx MoOx closed this May 28, 2014
@necolas
Copy link
Contributor

necolas commented May 28, 2014

There are some good consistency changes in this PR though :)

@MoOx
Copy link
Contributor Author

MoOx commented May 28, 2014

I just copy your work @necolas. I'm not the smart guy here.

@joakimbeng
Copy link
Member

Sorry for the late response, been traveling..
@jonathanong I've added the correct user as owner to rework-calc on npm now, it's the "jongleberry" account right?

@jonathanong
Copy link
Contributor

yeah. seeing if i can just delete it or transfer or it or something. switching users on npm is a pain in the ass.

MoOx added a commit that referenced this pull request Jun 20, 2014
Just a PR following my stupid
#3

I’ve refactored the code a little bit (& also use rework-visit which is
more bullet proof) like rework-vars.
I quickly refactor tests to make it more understandable.
I add some error reporting like we have in rework-vars.

Btw, maybe we can expand the use of balanced-match to rewrite other
parts of the plugin.
MoOx added a commit that referenced this pull request Jun 21, 2014
Just a PR following my stupid
#3

I’ve refactored the code a little bit (& also use rework-visit which is
more bullet proof) like rework-vars.
I quickly refactor tests to make it more understandable.
I add some error reporting like we have in rework-vars.

Btw, maybe we can expand the use of balanced-match to rewrite other
parts of the plugin.
@MoOx MoOx deleted the preserve branch June 21, 2014 05:38
@MoOx
Copy link
Contributor Author

MoOx commented Jun 21, 2014

Can someone add me as a owner on rework-calc so I can ship 1.0.0 ? I think it's time to (checkout latest commits) :)

@joakimbeng
Copy link
Member

@MoOx there you go :)

@MoOx
Copy link
Contributor Author

MoOx commented Jun 25, 2014

Great, thanks. v1.0.0 has been published !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants