search

meiersi / blaze-react

A blaze-html style ReactJS binding for Haskell using GHCJS
MIT License
107 stars 15 forks source link

Remove SVG attributes from Html5 attributes #13

Closed roelvandijk closed 9 years ago

roelvandijk commented 9 years ago

I checked the HTML 5.1 draft to see whether these were valid attributes. They appear to be only for SVG.

basvandijk commented 9 years ago

I added another commit to LumiGuide master that fixes the style attribute.

Previously the style attribute was of type:

style :: AttributeValue -> Attribute ev

This isn't correct in react since the value needs to be a Javascript object.

I fixed this by changing the type to:

style :: Json.Object -> Attribute ev

where a Json.Object is a HashMap Text Value.

asayers commented 9 years ago

@roelvandijk: Seems reasonable. We should probably make sure that Text.Blaze.Svg.Attributes contains all the attributes you removed from Text.Blaze.Html.Attributes.

@basvandijk: Yeah, this was a known issue (https://github.com/meiersi/blaze-react/issues/5) - thanks for fixing it. Is there a reason to use Aeson.Object rather than Map String String? I think the latter has some advantages:

  1. It doesn't pull in aeson and attoparsec.
  2. It prevents users from using arrays or objects as the values. For the use-case of the style attribute, this is clearly correct.
  3. It prevents users from using numbers as the values. React allows you to use numeric values, but it does a key-dependent stringification - yuck. I think it would be better to force users to do the string conversion explicitly.

What do you think? Eventually we'll probably want something like this:

myHtml = H.div ! A.style
    [ backgroundColor (RGB 27 32  70)
    , position Absolute
    , top 0
    , font ["helvetica", "ariel", "sans-serif"]
    ] $ ...
basvandijk commented 9 years ago

What do you think?

Yes, Map String String or Map Text Text definitely makes more sense. A proper CSS DSL (maybe even clay) would be even nicer.

I'm currently a bit pressed for time to fix this (big deadline for next Friday). @asayers, do you by any chance have time to improve upon this?

asayers commented 9 years ago

Sure. I'll do it tomorrow.

I had a quick look at Clay to see if we could use it. It's intended for generating stylesheets, rather than inline styles, so you bind styles to selectors, rather than just listing them. I'll have to look into how easily we can separate the style-definition part of the library from the selector-definition part.

basvandijk commented 9 years ago

I had a quick look at Clay to see if we could use it. It's intended for generating stylesheets, rather than inline styles, so you bind styles to selectors, rather than just listing them. I'll have to look into how easily we can separate the style-definition part of the library from the selector-definition part.

Yes I noticed that too. @sebastiaanvisser would it be hard to separate (type-wise) the style-definitions from the selector-definitions?

sebastiaanvisser commented 9 years ago

@basvandijk The obvious (with obvious I mean easy) thing to me seems to change the selector semigroup into a monoid and allow empty selectors that can be used inside style attributes.

Separating the types will be harder, because in Clay styles are, by design, hierarchical. Meaning you can always nest styles, creating something that doesn't fit within a style attribute. Of course you can always add a new module to Clay for inline styles that is just a flat list (or writer monad for neater syntax) of properties without hierarchy (Key (), Value).

The renderInline function would just be something like render . addEmptySelector.

asayers commented 9 years ago

For our purposes we would also need a version of render which returns something like HashMap Text Text, rather than just Text. It would be really cool though.

asayers commented 9 years ago

@roelvandijk I'm resistant to adding offsetX and offsetY. These values are inconsistent across browsers, and not even supported on firefox. I haven't tested it, but I expect that after this patch programs which use onMouseMove will throw errors on Firefox.

roelvandijk commented 9 years ago

@asayers Yes, I just noticed a crash when testing on Firefox. It didn't mean for this commit to end up in a pull request. Tomorrow I'll test a variant that should work equally well on different browsers.

I'm a bit stressed for time and needed the mouse position relative to the targeted container. This only seems to work in Chrome, which is not very satisfactory.

sebastiaanvisser commented 9 years ago

@asayers That should be even easier I guess, no need to go into the final render step. Feel free to give it a try if you like :)

What is the reason for having the Hashmap instead of fully rendering it to Text, you want to do further processing?

asayers commented 9 years ago

@sebastiaanvisser We want to serialise the HashMap as a javascript object and pass it to ReactJS, which expects inline styles to be expressed in this way. I guess they think it makes a nicer API.

asayers commented 9 years ago

@basvandijk @roelvandijk I merged in the first two commits only and made some of my own changes. This means that we've now diverged, but hopefully it shouldn't be a problem for you to rebase on top of meiersi:master.

basvandijk commented 9 years ago

Thanks Alex that looks good. Op 15 jan. 2015 03:17 schreef "Alex Sayers" notifications@github.com:

@basvandijk https://github.com/basvandijk @roelvandijk https://github.com/roelvandijk I merged in the first two commits only and made some of my own changes. This means that we've now diverged, but hopefully it shouldn't be a problem for you to rebase on top of meiersi:master.

— Reply to this email directly or view it on GitHub https://github.com/meiersi/blaze-react/pull/13#issuecomment-70029913.