chandu0101 / scalajs-react-components

Reusable scalajs-react components
http://chandu0101.github.io/sjrc/
Apache License 2.0
163 stars 65 forks source link

Material ui 0.15 #68

Closed oyvindberg closed 8 years ago

oyvindberg commented 8 years ago

keep rolling

jshin47 commented 8 years ago

May I have access to this pull request? I don't know if that makes sense but I kind of need to do this anyway

jshin47 commented 8 years ago

Nevermind lol I realized this is not your repository

raul782 commented 8 years ago

@oyvindberg I was testing this PR, and I noticed that the demo has a lot of warnings for components that have React children elements, We can fix it by adding auto-incremental numeric keys, unless there is a better solution.

oyvindberg commented 8 years ago

@raul782 Yeah, I have been ignoring those warnings, it's a general react/scalajs-react issue that we implicitly convert arrays of elements such that react wants those keys.

The reason why i havent done anything about it is that it defeats the purpose of keys in the first place. They are used as stable keys so react can do optimizations. Using for example indices would not (in the general case) provide stable keys. At least thats how i understand it.

For the demo we could do it, PR welcome if you want to look into it :)

chandu0101 commented 8 years ago

@oyvindberg if children element is not an array we don't need to provide keys! i guess there is an issue our side :s

@js.native
@JSName("React")
object ReactJS extends js.Object {
  def createElement(ctor: js.Any, props: js.Object, children: ReactNode*): ReactElement = js.native
}

use this to create elements and see if react complaining about warnings..

oyvindberg commented 8 years ago

It's this issue, isnt it? https://github.com/japgolly/scalajs-react/issues/206

chandu0101 commented 8 years ago

looks like it is :)

raul782 commented 8 years ago

@chandu0101 It's not clear to me, where we should replace object ReactJS, could you elaborate on this. (I'm just starting with scalajs and I'm trying to understand through the docs/code only found a reference in scalajs-react as React -> createElement ) From this issue japgolly/scalajs-react#206, I've managed to do a few changes on the Component CodeExample and wrap the children elements in a Seq,

object CodeExample {
...
def apply(code:     String,
            title:    String,
            ref:      js.UndefOr[String] = "",
            key:      js.Any = {})
           (children: ReactNode*) =
    component.set(key, ref)(Props(code,title), children:_*)
}

For example for:

object MuiAvatarDemo {
...
  val component = ReactComponentB[Unit]("MuiAvatarDemo")
    .render(P => {
      <.div(
        CodeExample(code, "MuiAvatar")(
          Seq(
            MuiAvatar(backgroundColor = colors.grey700, color = colors.deepPurple200, icon = SvgIcons.ActionGrade()())(),
            MuiAvatar(size = 120, backgroundColor = colors.lime600)("Ø"),
            MuiAvatar()("one"),
            MuiAvatar()("two"),
            MuiAvatar(backgroundColor = colors.red400)(SvgIcons.ActionFace()())
          ):_*
        )
      )
    }).build
...
}

While this removes the warnings, I believe there might be a more elegant solution.

Also when trying to apply this to all the components, I arrived to MuiButtonsDemo that contains MuiTabs, since the apply method is using js.Dynamic:

val f = React.asInstanceOf[js.Dynamic].createFactory(Mui.Tabs)
...
f(props, children.toJsArray).asInstanceOf[ReactComponentU_]

I can't use the same strategy: f(props, children:_*).asInstanceOf[ReactComponentU_] Getting a compilation error here, is there a way to overcome this issue, I'm looking at scalajs-react js-components to better understand what alternatives exist.

chandu0101 commented 8 years ago

@raul782

val f = React.asInstanceOf[js.Dynamic].createFactory(Mui.Tabs)
...
f(props, children.toJsArray)

this is same as

ReactJS.createElement(Mui.Tabs,props,children :_*)
timothyklim commented 8 years ago

Is there a roadmap or date when 0.15 be released? Thanks.

oyvindberg commented 8 years ago

Good news people, i finally found some time to wrap this up! :)