-
Notifications
You must be signed in to change notification settings - Fork 250
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
SVG Gradient Generation Support #33
base: master
Are you sure you want to change the base?
Conversation
Hi Mike, Thanks for the code. I think SVG support is important not so much for quality but for IE9 and Opera that do not have equivalent working CSS3 gradients. For <= IE8 I personally prefer PIE.htc and IE10 has the -ms- vendor prefixed gradients. I did some initial SVG support some time ago for my radial gradients patch at https://github.com/visionmedia/nib/pull/12/files#L2R321 and https://github.com/visionmedia/nib/pull/12/files#L5R78. That codes more of a working proof of concept for radial gradients and not ready for merging though. I also wasn't sure of the best way to go with it. My demo just renders the SVG string with stylus, then encodes it with an exported function as a bit of a nasty hack. Using Jade to render the XML is an interesting idea. The only question is if the dependency on Jade is worth it for the amount of XML to generate. Whatever the solution is I think the Gradient object needs to be broken up eventually, although that may be out of scope of this issue. The SVG mechanism should be the same for linear and radial gradients, and share common code if possible. Possibly something like I also think web browser support options should determine if SVG is rendered or not, instead of calling multiple mixins. Deferring to mixins is fine, like the current linear gradient mixin defers to a canvas image mixin. Currently the web browser support options consists of "config" variables like Is #31 a duplicate of this issue ? A heads up that you missed removing a debug Great to see more contributions for nib so keep up the good work =) Look forward to hearing your thoughts on the points above. Cheers |
Thanks for the feedback. Yeah I was on the fence with including Jade, but it was already a dev dependency, and I figured it cleaned up the XML rendering logic a bit so I included it. But yeah, you are right, it probably isn't worth including it. I'm not so sure that browser support options should determine if SVG is rendered or not. Currently, PNG is higher quality than SVG. However, as SVG is a lossless format, it is the better option when the size has to be scaled to fit the content and that size is potentially large. For these reasons, I think that only the user can determine if SVG is an appropriate option, based on size, browsers, quality of the needed gradient. The current gradient code was very much hard coded towards linear gradients and the canvas implementation, so I kinda wanted to just get the pull request out to get feedback on how the core contributors though this feature should be integrated in, or if at all. Yeah, I noticed after I pushed the pull request I had the console.log in statement in SORRY! I was going to push a new commit tomorrow to remove it, but hadn't had time. 31 is a duplicate issue I guess. Github makes pull requests an issue. I logged issue 31 to see if anyone would comment on it for initial feedback or direction. I didnt' hear anything, so I just started coding. As far as configuration goes, my opinion is that it would make more sense to specify when create specific gradients if you want them to be generated with CSS, PNG inline or SVG inline. Depending on the gradient and desired browser support, any one implementation could be optimal. Thanks! |
Good points on browser support and configuration options. I think you are right since as you say SVG and PNG can both cover some browsers lacking support for CSS, only the user can decide which is optimal. However it is configured the essential point is the user should only need to call a single top-level gradient mixin once per gradient definition to avoid duplicating gradient information (such as colors, stops etc). Something similar to the Yep the current code is very much linear gradient and canvas oriented. It is likely you could get this merged before radial gradients which would put that issue out of scope of this changeset and my responsibility to refactor when I finally merge radial gradient support. When I do I'll aim to keep the mechanisms the same, share common code, etc. Thanks |
Are multiple formats ever going to be needed? If I'm using PNG, I can't see a reason that CSS3 or SVG would be needed. Likewise, off the top of my head I think that every browser that supports SVG won't need CSS3 gradients in place. I think that the linear-gradient declaration should just take a single format to be used, with a default if none is specified.
So if thats cool, then I think we're landing on the following changes to this pull request (correct me if I'm wrong).
Also:
|
This is useful for cross-browser support e.g. Also I plan to add PIE.htc support soon, another common cross browser strategy might be the list "css svg pie" (note list should not infer any ordering of properties). With the combination of a config list,
https://github.com/LearnBoost/stylus/blob/master/docs/kwargs.md
Thanks! |
cool I'll try and take a closer look soon thanks man |
Cool, if we are using keyword arguments, can I propose we also add opacity and a vector direction option? I know in svg for sure it's trivial to create gradients that are not parallel to the x or y axis |
We might be able to apply a filter to the SVG gradient to give it a smoother appearance. The SVG gradients appearance only seems to be rough when the size is very small; such as when the gradient is applied to a button. However, from what I've seen, at those small sizes, the PNG images are smaller anyway, and look better so maybe its not worth it. I'll investigate. |
Sure that would be cool. W3C gradient syntax also supports direction in degrees (moz, new webkit etc). Old webkit syntax doesn't. It would probably also be possible to replicate for the PNGs, but feel free to leave that for later implementation. It shouldn't be an issue that different formats have different levels of support for the extra options. |
Also I've been thinking more about the use of Jade. If its not too difficult moving the XML to an external |
Yeah, I thought about that, but loading in the file would be an async operation. Should I just read in the jade file sync? The stylesheets should only be compiled in development so it shouldnt' be that big of a deal I wouldn't think. Or is there a better way to do it? |
External templates is a good idea as when I get to the background noise generator.. I"ll have additional SVG templates for those filters as well. |
Good point. I don't think you can do an async call in the methods that get called from stylus expecting a return value obviously. Maybe you could load the template once uncompiled into a var when the JS is first loaded, which is before the stylus starts calling methods. Then just compile the template per call from stylus. It is possible to compile nib-based stylus in production at runtime without ever storing the compiled CSS, but this is usually cached for performance reasons anyway. This is how I usually use nib. The code for Express/Connect is like: app.use(stylus.middleware({
src: __dirname + '/public'
, dest: __dirname + '/public'
, compile: function(str, path) {
return stylus(str)
.include(require('nib').path)
.set('filename', path)
.set('warn', true)
.set('compress', false);
}
})); |
yeah we can just load / compile the template and cache the function |
I think I'm leaning towards making the size be a named argument as well, being its only needed on 1 of the 3 formats. |
@superstructor have you had a chance to look at this yet? |
Thanks for the updates and sorry for the slow reply. Looking good. Having the SVG in an external file is nice.
If you do decide to do so it would be nice to maintain backwards compatibility with the current syntax. Cheers |
So, I take back that comment on the 'size' named arg. I think it is fine the way it is. The gradient constructor just needs to be modified to not take a size. I think the size should be set on the gradient object only if one exists. I also think we should honor size on SVG images if it is passed in. Scaling a SVG gradient to always fit the container is not always ideal. Take for example if I want to fade from light gray to solid white in 200px on the body. If the SVG always scales to content, I'd have to use a pseudo class to achieve that, when SVG can easily be limited to that size. Only 1 dimension should suffice for the SVG image. The width of the image parallel to the gradient vector will be the length specified by size, and the perpendicular axis can scale to 100%. Of course, a gradient vector that is not parallel to the standard X/Y axis would throw this logic off a bit, but its just a thought. |
Good points. I agree. |
Ok, I pushed the updates I mentioned for sizing. This code should be good to go. I want to mention though, that combing the size and start make a huge headache. My vote is to completely move size to a named argument, which would simplify things a lot, but would be a non-passive change. I apologize for the length of this pull request. Please let me know if you have any corrections or changes. Thanks guys! |
* Default vendor prefixes. | ||
*/ | ||
|
||
gradient-formats = png svg css |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
?=
only assigns if not already defined
@visionmedia what can we do to help to get this merged ? Once this is done I can finish #12 which has been open for a year ;-) Cheers |
This will need to be re-written on top of #94. I've closed #12 (radial) as will do a new pull request for that when #94 is finished. If you don't have time to re-write this Mike @mhemesath I'm happy to take on both linear and radial SVG/canvas support. Cheers =) |
@superstructor I may have time this weekend to get a start on this. I regularly use nib in a Rails project, so I should prioritize this. |
No progress here? |
Here's a start on SVG support for gradient image generation.
Changes
This is a rough start. Wasn't 100% confident on what direction to take this. Let me know what you think. Unfortunately, the quality is the same as CSS3 from what I could see in Chrome.