Rythm.js icon indicating copy to clipboard operation
Rythm.js copied to clipboard

SVG opacity and fill

Open LaikaFusion opened this issue 7 years ago • 6 comments

Forgive me I'm new at this so I hope I did everything right. This is for #68 and #69

Since the dances pretty much had the logic in them already it seemed better to detect the element calling it and then handle the exact property needed rather then duplicate.

I did test these, and they do work with one caveat. It needs to be on a <svg></svg> tag. Applying styles to <Object> tags doesn't effect the underlying svg.

A good test image would be the font awesome music note svg

LaikaFusion avatar Oct 11 '18 02:10 LaikaFusion

Thanks for your contribution. I'll review it in the incoming days :) !

Okazari avatar Oct 11 '18 08:10 Okazari

Hey. Sorry for the delay. I've been really busy theses days. Gonna give it a look asap to merge it before the end of the month

Okazari avatar Oct 28 '18 10:10 Okazari

I'll take a look at this a bit later when I have some time.

LaikaFusion avatar Oct 29 '18 16:10 LaikaFusion

I'm a little confused right now. By what you're asking here.

You mention p tags, and in this context I'm assuming you mean a p tag inside of two svg ones? If that's the case the fill styling will also apply to that

tag and all others. If you didn't want it to you'd have to just apply it to the internal elements you wanted. I'm not sure how this is different then applying it to a div containing text vs a span inside the div for only certain text.

The other way I can read that first paragraph is that you want this to support something like using a background to show the svg. Or an tag with src set to svg. This isn't going to work for fill because SVGs need to be inline for css to apply to them.

You could use css filter for this actually, but that feels out of scope of this particular PR as it would work with all images. (This may be a good thing to put on the issue tracker actually).

For the second paragraph are you requesting I make two separate functions for SVG and non SVG versions seems like it'd be an unnecessary duplication of code. Why have colorSVG and color dance that both use the same calc function but the only difference being the element style property.

LaikaFusion avatar Nov 02 '18 21:11 LaikaFusion

Hello Laïka and thanks again for your time.

I will try to summary what my concern is : By SVG elements i didnt mean sticktly SVG tags. There is many more SVG tags (https://developer.mozilla.org/en-US/docs/Web/SVG/Element)

I want my API to be compatible with any svg tag and there is two options :

  • We find a way to know that the targeted element is an element created in the svg namespace and use this instead of the verification of svg tag on your current code.

  • We provide two dance using the same algorithm for the dance (but we factorise that) so that it become the user responsibility to use the right type of dance according to its element type

Is that clearer ?

PS: sorry i missclicked on the close and comment button

Okazari avatar Nov 04 '18 07:11 Okazari

Hey @LaikaFusion, are you still up to take care of this dance ? :smile:

Okazari avatar Apr 08 '19 13:04 Okazari