Skip to content

Conversation

ytbryan
Copy link
Contributor

@ytbryan ytbryan commented Feb 3, 2018

This is merged at #1255

hello.setAttribute("data-controller", "hello")
hello.setAttribute("data-target", "hello.output")
document.body.appendChild(hello)
})
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Creating an element with JavaScript isn't a very good introduction to Stimulus since you'll almost always be working with server-rendered HTML. Let's ditch this file and start people out right by appending the setup code to app/javascript/packs/application.js:

import { Application } from "stimulus"
import { definitionsFromContext } from "stimulus/webpack-helpers"

const application = Application.start()
const context = require.context("controllers", true, /\.js$/)
application.load(definitionsFromContext(context))

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👌 ok, ready for another look.

Appended the setup code to app/javascript/packs/application.js and ditched the previous file.

guilleiguaran and others added 4 commits February 4, 2018 13:58
…egrity-config

Add line break after check_yarn_integrity template config
From CI "Use the new Ruby 1.9 hash syntax.
inject_into_file "#{Webpacker.config.source_entry_path}/application.js", :after => "console.log('Hello World from Webpacker')" do"
@@ -0,0 +1,12 @@
say "Appending Stimulus setup code to #{Webpacker.config.source_entry_path}/application.js"
inject_into_file "#{Webpacker.config.source_entry_path}/application.js", after: "console.log('Hello World from Webpacker')" do
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Injecting after console.log('Hello World from Webpacker') feels kind of brittle. Maybe just append to the end?

@@ -0,0 +1,8 @@


Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's rename this file to application.js since that's the file it ends up in. And move this leading whitespace to the injection.

import { Controller } from "stimulus"

export default class extends Controller {
static targets = [ "output" ]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add a \n

"webpacker:install:erb" => "Installs Erb loader with an example",
"webpacker:install:coffee" => "Installs CoffeeScript loader with an example",
"webpacker:install:typescript" => "Installs Typescript loader with an example"

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✂️

static targets = [ "output" ]
connect() {
this.outputTarget.textContent =
`Hello Stimulus!`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need for the \n here or the interpolated string: this.outputTarget.textContent = 'Hello Stimulus!'

javan and others added 8 commits February 5, 2018 15:53
`warn` messages are silenced when `$VERBOSE=nil` (verbosity level 0). We're well beyond casual warning territory here and refusing to boot with an error.
this.outputTarget.textContent = 'Hello Stimulus!'
@ytbryan
Copy link
Contributor Author

ytbryan commented Feb 6, 2018

👌 ready

import { definitionsFromContext } from "stimulus/webpack-helpers"

const application = Application.start()
const context = require.context("./controllers", true, /.js$/)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does "./controllers" work? That's not the relative path to the controllers directory. When I tested, "controllers" worked as expected since webpacker adds all root app/javascript/ directories to the load path.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for pointing this out. I uninstalled the old webpacker and now, it's working for me 🤷‍♂️

@@ -0,0 +1,12 @@
// Visit The Stimulus Handbook for more details
// https://stimulusjs.org/handbook/introduction
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be nice to show the HTML this controller expects in the comments here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good idea!

Copy link

@Othmano2399 Othmano2399 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this Related??

@javan
Copy link
Contributor

javan commented Feb 6, 2018

Can you update the PR description, please? Just need to describe the webpacker changes and link to Stimulus where appropriate.

// Visit The Stimulus Handbook for more details
// https://stimulusjs.org/handbook/introduction
//
// This controller is expecting the following HTML. Paste it with the javascript_pack_tag helper.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about:

// This example controller works with specially annotated HTML like:
//
// <div data-controller="hello>
//   <h1 data-target="hello.output"></h1>
// </div>

static targets = [ "output" ]

connect() {
this.outputTarget.textContent = 'Hello Stimulus!'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

-'Hello Stimulus!'
+'Hello, Stimulus!'

// <div data-controller="hello" data-target="hello.output">
// </div>


Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✂️extra newline

@ytbryan
Copy link
Contributor Author

ytbryan commented Feb 6, 2018

Updated description and pushed in the updated commit. If there are new reviews, I will push in tomorrow morning. Thanks for reviewing!

@rails rails deleted a comment from Othmano2399 Feb 6, 2018
end

say "Creating controllers directory"
directory "#{__dir__}/examples/stimulus/controllers", "#{Webpacker.config.source_entry_path}/controllers"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should create an app/javascript/controllers/ directory here, not app/javascript/packs/controllers/. So use Webpacker.config.source_path instead of Webpacker.config.source_entry_path.

That's why #1244 (comment) worked for me and not you.

@javan
Copy link
Contributor

javan commented Feb 6, 2018

Once #1244 (comment) is fixed, please squash your commits and then we're good to go.

webpacker:install:stimulus

added controllers directory and hello_controller.js

HELLO stimulus!

Wrapping it up with stimulus.rb

include stimulus in README.md

Remove hello_stimulus.js

Append setup code to application.js

👮‍♂️Satisfy robocop

From CI "Use the new Ruby 1.9 hash syntax.
inject_into_file "#{Webpacker.config.source_entry_path}/application.js", :after => "console.log('Hello World from Webpacker')" do"

use append_to_file instead of inject_into_file
add whitespace

rename setup.js to application.js
remove whitespace

added a \n
this.outputTarget.textContent = 'Hello Stimulus!'

✂️

✂️ to appease robocop

fix: cannot find module "controllers"

revert back to "controllers"

Added the expectant HTML in comments. 🤰

✂️, Edit comments, and changed to 'Hello, Stimulus!'

Use source_path instead of source_entry_path

missing "
@ytbryan
Copy link
Contributor Author

ytbryan commented Feb 7, 2018

Good to go 👍🏻

@javan
Copy link
Contributor

javan commented Feb 7, 2018

It looks like you merged master. Can you rebase and squash instead, please? There's a linked guide in my last comment.

@@ -11,6 +11,7 @@ tasks = {
"webpacker:install:vue" => "Installs and setup example Vue component",
"webpacker:install:angular" => "Installs and setup example Angular component",
"webpacker:install:elm" => "Installs and setup example Elm component",
"webpacker:install:stimulus" => "Install and setup Stimulus component",
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

-"Install and setup Stimulus component
+"Installs and setup example Stimulus component

Is more consistent to the rest od task description.

@ytbryan
Copy link
Contributor Author

ytbryan commented Feb 8, 2018

@javan Sorry for the trouble. Will this work instead? #1255

I guess the problem lies with me working on my master branch, dork


update: I tried to rebase and squash again but couldn't get it to work. do you think merging #1255 is easier? sorry about this.

@javan
Copy link
Contributor

javan commented Feb 8, 2018

#1255

@javan javan closed this Feb 8, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants