search

ochrons / scalajs-spa-tutorial

Tutorial for creating a simple Single Page Application in ScalaJS
Apache License 2.0
672 stars 229 forks source link

Check scalajs-react compatibility #27

Closed ochrons closed 8 years ago

ochrons commented 8 years ago

@japgolly, could you go over the React code in the "diode-integration" branch and check that it uses scalajs-react TheWayItWasMeantToBeUsed :)

Wouldn't want to set a bad example for the upcoming generations of Scala.js developers!

japgolly commented 8 years ago

Happy to!

Can't do it today cos I'm in the middle of a Scala-based nightmare and need my wits to escape. Let me get back to you.

On 27 November 2015 at 07:25, Otto Chrons notifications@github.com wrote:

@japgolly https://github.com/japgolly, could you go over the React code in the "diode-integration" branch and check that it uses scalajs-react TheWayItWasMeantToBeUsed :)

Wouldn't want to set a bad example for the upcoming generations of Scala.js developers!

— Reply to this email directly or view it on GitHub https://github.com/ochrons/scalajs-spa-tutorial/issues/27.

ochrons commented 8 years ago

Planning to release this on Monday with Diode, so no pressure or anything :smile:

japgolly commented 8 years ago

Cool, I plan to look at this tonight my time, which is Sun morning your time, so we should be good :)

japgolly commented 8 years ago

Ok I had a look and made some changes here: https://github.com/japgolly/scalajs-spa-tutorial/commit/57a718f35de8459247669020d462ff48dedbe5f5

Note: I tried not to be pedantic and mess around with just style changes (except for a few that I think are important); also I didn't turn the brain on very much to really understand the logic. Also I upgraded scalajs-react to 0.10.2-SNAPSHOT which I'll release properly within 24 hours.

I saw a few instances of .copy(.copy stuff which becomes much nicer and clear with Monocle so I made another commit to demonstrate that - it works quite nicely with modState. Like lines 93 & 102 in TODO. https://github.com/japgolly/scalajs-spa-tutorial/commit/68afbbc87cd6a58f7bed46bd9ceec0cd0210e6b3

ochrons commented 8 years ago

Thanks, the changes look very nice! Didn't know that <.input.text was a thing :)

As for Monocle, I considered it earlier but I think it's a bit too much for this tutorial. Especially now that Diode also brings its own "lens" concept to the project.

ochrons commented 8 years ago

Can you make a PR for the 57a718f commit and I'll integrate it.

ochrons commented 8 years ago

Ah, nevermind, I was able to cherrypick it myself!

japgolly commented 8 years ago

Ah I was just about to do this. Good cherry pickin'.