-
-
Notifications
You must be signed in to change notification settings - Fork 27
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 support #25
Svg support #25
Conversation
…rs/svgAttrs.scala:8: Attrs is already defined as trait Attrs
recreate svg branch
finished this really long svg type def..
README.md
Outdated
## Community & Contributing | ||
|
||
### updating: | ||
Currently we are working to add support for svg at svg branch ,help very welcomed! |
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.
This is not needed given what's in master and in #26
@@ -86,6 +86,10 @@ package object defs { | |||
type EmbedTags[T[_ <: dom.Element]] = generic.defs.tags.EmbedTags[T, dom.Element, dom.html.Image, dom.html.IFrame, dom.html.Embed, dom.html.Object, dom.html.Param, dom.html.Video, dom.html.Audio, dom.html.Source, dom.html.Track, dom.html.Canvas, dom.html.Map, dom.html.Area] | |||
type TableTags[T[_ <: dom.Element]] = generic.defs.tags.TableTags[T, dom.Element, dom.html.Table, dom.html.TableCaption, dom.html.TableCol, dom.html.TableSection, dom.html.TableRow, dom.html.TableCell, dom.html.TableHeaderCell] | |||
type MiscTags[T[_ <: dom.Element]] = generic.defs.tags.MiscTags[T, dom.Element, dom.html.Title, dom.html.Style, dom.html.Element, dom.html.Quote, dom.html.Progress, dom.html.Menu] | |||
import org.scalajs.dom._ | |||
import svg._ |
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.
please don't use those extra imports – instead, spell out dom.svg.Type
everywhere instead of just Type
– yes, even in this huge list. It's just much easier to text search, and matches the style in the rest of the definitions here.
@@ -86,6 +86,10 @@ package object defs { | |||
type EmbedTags[T[_ <: dom.Element]] = generic.defs.tags.EmbedTags[T, dom.Element, dom.html.Image, dom.html.IFrame, dom.html.Embed, dom.html.Object, dom.html.Param, dom.html.Video, dom.html.Audio, dom.html.Source, dom.html.Track, dom.html.Canvas, dom.html.Map, dom.html.Area] | |||
type TableTags[T[_ <: dom.Element]] = generic.defs.tags.TableTags[T, dom.Element, dom.html.Table, dom.html.TableCaption, dom.html.TableCol, dom.html.TableSection, dom.html.TableRow, dom.html.TableCell, dom.html.TableHeaderCell] | |||
type MiscTags[T[_ <: dom.Element]] = generic.defs.tags.MiscTags[T, dom.Element, dom.html.Title, dom.html.Style, dom.html.Element, dom.html.Quote, dom.html.Progress, dom.html.Menu] | |||
import org.scalajs.dom._ | |||
import svg._ | |||
type svgTags[T[_ <: dom.Element]]=generic.defs.tags.SvgTags[T,dom.Element,svg.Element,Circle,ClipPath,Defs,Desc,Ellipse,FEBlend,FEColorMatrix,ComponentTransferFunction,FEComposite,FEConvolveMatrix,FEDiffuseLighting,FEDisplacementMap,FEDiffuseLighting,FEFlood,FEFuncA,FEFuncB,FEFuncG,FEFuncR,FEGaussianBlur,FEImage,FEMerge,FEMergeNode,FEMorphology,FEOffset,FEPointLight,FESpecularLighting,FESpecularLighting,FETile,FETurbulence,Filter,G,Image,Line,LinearGradient,Marker,Mask,Metadata,Path,Pattern,Polygon,Polyline,RadialGradient,RectElement,SVG,Stop,SVG,Switch,Symbol,svg.Text,TextPath,TSpan,Use,View] |
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.
Pls capitalize the first S so that this type's name is consistent with other type names above
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.
Formatting: add spaces around =
, and put each type param on a new line
SvgRectElement <: SvgElement, | ||
Svg <: SvgElement, | ||
SvgStop <: SvgElement, | ||
SvgSVG <: SvgElement, |
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.
This type param seems to be unused? There's another Svg two lines above this
|
||
import com.raquo.domtypes.generic.builders.TagBuilder | ||
|
||
trait SvgTags[T[_ <: DomElement], DomElement, SvgElement <: DomElement, |
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.
T[_ <: DomElement]
should be T[_ <: SvgElement]
, I think, and it doesn't seem like we need the DomElement
type param at all.
* | ||
* MDN | ||
*/ | ||
lazy val altGlyph:T[SvgElement] = tag("altGlyph") |
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.
Formatting: Pls add a space between :
and T
in all these lazy vals to be consistent with other traits like this
* | ||
* MDN | ||
*/ | ||
lazy val `missing-glyph`:T[SvgElement] = tag("missing-glyph") |
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.
We should not be using backticks. Tags with dashed should be camelCased instead. (same for other tags)
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.
That's true,gonna do that later with regex after initial test
import com.raquo.domtypes.generic.codecs.Codec | ||
|
||
/** | ||
* This class represents an HTML Element Attribute. Meaning the key that can be set, not the whole a key-value pair. |
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.
Update "HTML" to "SVG"
import com.raquo.domtypes.generic.builders.AttrBuilder | ||
import com.raquo.domtypes.generic.codecs.{BooleanAsOnOffStringCodec, BooleanAsTrueFalseStringCodec} | ||
|
||
/** @tparam A svg Attribute, canonically [[com.raquo.domtypes.generic.keys.SvgAttr]] */ |
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.
Formatiing: svg should be capitalized ("SVG") in the comment
CI gives error due to that some attr name is the same as element name .. [error] /home/travis/build/raquo/scala-dom-types/js/src/test/scala/com/raquo/domtypes/CompileTest.scala:76: object svg inherits conflicting members: /**
* The clip-path attribute bind the element is applied to with a given <clipPath> element
* As a presentation attribute, it also can be used as a property directly inside a CSS stylesheet
*
* Value <FuncIRI> | none | inherit
*
* MDN
*/
lazy val clipPath = stringAttr("clip-path") Should we namespace them ? Or provide alternative name? |
Yep, that's exactly what CompileTest is for, nice. Unless we know that the BTW you don't need to wait for CI, you can run the tests locally with |
I have added postfix "Attr" for the attrs and now it passed.Thank for the advice! Maybe it's time to use dom builder to test svg rendering? |
For DOM Builder work, we'll need a sample SVG file I described in this comment: #20 (comment) |
Do you mean something like |
No I mean we need such example in plain natural SVG (that is <svg blah=""
etc.>). It should contain attributes of all possible different datatypes
(e.g. numbers, strings, booleans)
…--
Nikita
On Mar 1, 2018 5:39 PM, "doofin" ***@***.***> wrote:
Do you mean something like svg(circle(rx:=5,ry:=5)) ? But this syntax
seems only available at dom builder which is yet to be finished..
—
You are receiving this because your review was requested.
Reply to this email directly, view it on GitHub
<#25 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAggsHzKZqhMZtl05ENWMkkZX8v17WG4ks5taKK1gaJpZM4SR0S6>
.
|
So you want to make dom builder render to string and test their equality,right? Where should i put these svg file? |
Plenty of thanks to lihaoyi (http://www.lihaoyi.com/scalatags/#scalatags.TextSVG) |
Cool, looks like it should work. I'll try to get something going on DOM Builder side on the weekend. Sorry for the delay, very busy days now between work and various chores. |
No worry,after all we are doing this for fun.I would also become very busy soon due to that I got a great scala job about big data . Scala is quite popular these days in China,which is unimaginable! |
Also: - Give more specific types to SVG attrs - Various renamings for consistency - Formatting fixes for consistency - Address other minor code review comments
@doofin I almost finished integrating SVG into DOM Builder and Laminar. While I'm wrapping that up, could you please copy over ScalaTags/MDN comments to remaining SVG attrs? I should have my SDB / Laminar changes ready some time next week. Make sure to first pull the latest changes from this branch to avoid merge conflicts, as I changed the SvgAttrs file among others. |
@raquo which svg attrs are missing? I think they are all here now |
@doofin it's not the attributes themselves that are missing, but the MDN comments for them, starting with |
The comments are not present in scalaTags either..so I would have to get them from MDN site myself.I would be happy to write a parser to get them from web automatically but currently I am too busy to do that @raquo For reference : https://developer.mozilla.org/en-US/docs/Web/SVG/Attribute |
Ah I see, I didn't know they were missing from ScalaTags too. Definitely no
need to spend more time on that then. I'll keep #29 up and if anyone cares
enough they can do it.
Thanks for your help with SVG! I will publish v0.6 on the weekend. SDB and
Laminar stuff I already put in PRs, will be merging all that hopefully this
weekend too.
—
Best regards,
Nikita
…On Mon, Apr 9, 2018 at 8:24 PM, doofin ***@***.***> wrote:
It's not present in scalaTags either..so I would have to get them from MDN
site myself.I would be happy to write a parser to get them from web
automatically but currently I am too busy to do that
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#25 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAggsI1etv6nnLJhNF0OteDPLF7p7XeZks5tnCYBgaJpZM4SR0S6>
.
|
Thank you all for working on this PR! |
Cheers and thank everyone ! Finally we can do some very complex stuff with svg and reactive programming! |
Seems that we should move every svg thing here,which is idiom github workflow..
For reference:
Scalatags impl:
SVG tag to dom-type association in scalatags
https://github.com/lihaoyi/scalatags/blob/44cbd0602d40186d33d39b8d4a259f681d2e4a3b/scalatags/shared/src/main/scala/scalatags/generic/SvgTags.scala
Svg attrs https://github.com/lihaoyi/scalatags/blob/44cbd0602d40186d33d39b8d4a259f681d2e4a3b/scalatags/shared/src/main/scala/scalatags/generic/SvgAttrs.scala
svg/Attrs.scala
trait with typed listings of SVG attributes(created : https://github.com/raquo/scala-dom-types/blob/svg/shared/src/main/scala/com/raquo/domtypes/generic/defs/attrs/svgAttrs.scala )
svg/Tags.scala
(might need multiple files similar to how HTML tags are done)(created: https://github.com/raquo/scala-dom-types/blob/svg/shared/src/main/scala/com/raquo/domtypes/generic/defs/tags/SvgTags.scala )
jsdom/defs/package.scala
(https://github.com/raquo/scala-dom-types/blob/svg/js/src/main/scala/com/raquo/domtypes/jsdom/defs/package.scala)SvgAttr
(I don't think we should reuse Attr for that), and a canonical builder for it (seecanonical
dir). Don't want to accidentally use SVG attrs on HTML elements, and without Track parent tag of properties #13 not sure what else we can do. Not sure about this, and IIRC you're not using these concrete classes in Outwatch, so I guess I'll need to think about this some time later.(Don't understand this point,Currently I am reusing
lazy val accentHeight = stringAttr("accent-height")
,will there be problems?)
Tag
class, I think no need to createSvgTag
, maybe Tag[SVGElement] would be enough. Not sure.(Currently as T[SvgElement])
fixes #20