-
Notifications
You must be signed in to change notification settings - Fork 26
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
Feedback: Your First App #12
Comments
Maybe also of interest: Those two passages gave me additional insight over the official documentation.
Both from Your first app. |
Hi 👋 Firstly, thanks for taking the time to do this! I haven't yet used choo but I want tooooo. I'm a noob who also has a new computer – so, I run the |
Hi @MelissaKaulfuss, thanks for the feedback! You've raised a good question, and I'm not 100% sure if the answer. Perhaps we could link to the npm docs, or a separate mini article (like an appendix) or perhaps we should decide our target reader may not have npm installed. I'd certainly welcome help on any of the above. In the meantime, I usually point people to c9.io, a website that gives you a full Linux VM in the cloud with a code editor on it. It comes with node and npm installed. Alternaticely the nodejs docs probably have the best info on installing. Another alternative is to use the bundled version of choo, which is just a script tag, no node required, though then we'd have to pull in extend too. |
I just read the tutorial. The only thing that was not clear to me was finding the 'onload' event on a div tag here
I'm not sure if that makes reference to a DOM event (not familiar with using onload on divs) , or a choo lifecycle event. Maybe adding a note with some clarification helps. |
Reading through the tutorial. The tagged template literals was a new thing for me - pretty cool! I have a small refactoring suggestion for current: addTodo: (data, state) => {
const newTodos = state.todos.slice()
newTodos.push(data)
return { todos: newTodos }
} suggested: addTodo: (data, state) =>
({ todos: [...state.todos, data] }) making it a one-liner. Thank you |
Is removing of (It was very easy to add). |
We could get away without const newTodo = extend(oldTodo, data.updates);
const newTodo = Object.assign({}, oldTodo, data.updates); |
@mickeyvip Not sure if adding more dependencies to ES6 features is a good idea for the handbook. In the choo docs they state that the only ES6 things needed are const, fat arrows and templated-strings which can be backported with ES2020. Using additional ES6 constructs may confuse beginners who think that ES2020 is enough. |
@hugozap , ES6 is used in the book, so I figured one might use more of its features. If I understand correctly, With that I totally understand the decision to stick to more Thank you. |
Hey folks just wanted to say thanks so much for your feedback! I've just added a link to npm in the docs, and I called out the |
Really enjoyed both chapters of the handbook so far, keep it up! A couple of typos:
|
I was able to follow the tutorial but didn't realize that the |
Heya, thanks for the feedback! - Could you perhaps create a PR for this? - On Fri, Nov 18, 2016 at 5:27 AM Malone Hedges [email protected]
|
Hi! Thanks for nice handbook. I've just played with it on my Mac and choo I had to replace suggested code for router:
to
since the suggested one failed with error Then I have had to replace reducer functions arguments order from If this changes were correct, I'll be glad to make an update to the documentation. |
Heya, yeah this changed in choo v4 - I'm sure the guide should be updated,
a PR would definitely be welcome! :D
…On Fri, Dec 16, 2016, 21:36 Nikolai Lopin ***@***.***> wrote:
Hi! Thanks for nice handbook. I've just played with it on my Mac and choo
4.0.1 and have faced with some obstacles.
I had to replace suggested code for router:
app.router((route) => [
route('/', view)
])
to
app.router([
'/', mainView
])
since the suggested one failed with error "AssertionError: sheet-router:
tree must be an array
Then I have had to replace reducer functions arguments order from (data,
state) to (state, data).
If this changes were correct, I'll be glad to make an update to the
documentation.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#12 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ACWleqZ1G2Dz0DvkNFHUSfTC2IvDb-zMks5rIvZmgaJpZM4JEW7Q>
.
|
The v4 changes should all be in there now, but please let us know if we've missed anything else, thanks! |
Nice, thanks Tim 🙏🎉
…On Tue, Dec 20, 2016, 12:33 Tim Wisniewski ***@***.***> wrote:
The v4 changes should all be in there now, but please let us know if we've
missed anything else, thanks!
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#12 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ACWleh0CxiVjyIKgnmTGu6ELa6tts8qIks5rJ70XgaJpZM4JEW7Q>
.
|
Hi, loving choo already, but your guide switches it's parameters order around in the guide! getTodos: (state, data, send, done) => { But then it switches around after the localStorage code: getTodos: (data, state, send, done) => { |
Actually, looking deeper now, the parameters are all over the place... There's even a line that looks like this: getTodos: (state, state, send, done) => { |
Hm, it looks like those have all been fixed in the code, but not reflected in the gitbook. @yoshuawuyts, does this repo have automated deploys working? |
Those might have stopped working - think I got ahead of myself with setting
up handbook.choo.io hah
…On Tue, Jan 17, 2017, 11:54 Tim Wisniewski ***@***.***> wrote:
Hm, it looks like those have all been fixed in the code, but not reflected
in the gitbook. @yoshuawuyts <https://github.com/yoshuawuyts>, does this
repo have automated deploys working?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#12 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ACWlej-NwwU9U6EHB7NV7wEiflFgKgAVks5rTJ3NgaJpZM4JEW7Q>
.
|
I dunno. To me this makes no sense. You never describe how to set up a project: package.json? npm install? What are the dependencies? How do you build it? etc. |
@rbiggs in the boilerplate section, there's an The build step is listed in the end of the rendering data section. What details were you expecting in the tutorial? |
Does the order of the params matter? Seems like they are ordered differently at the top before the refactor and work fine. Then at then end, if you keep that order, app stops showing the names in the DOM. when I switched data, state, to state data, the app functioned correctly. Is there something about the refactor that makes it so we have to change the order of the params in the reducers? Thank you. |
@idkjs it's a known issue. If you look at the source, you can see what it should look like. |
@JRJurman I had it working already. Just wondering what was going on with that. You said its known issue? What is the known issue? Is there an issue feed I can read? Thanks! |
@idkjs if you look at the messages above, it's something I brought up a few weeks ago. #12 (comment) I don't know what progress has been made since then... |
Really enjoying playing with choo! Something I noted while following the handbook was that it might be worth mentioning that void elements must be self-closed and the reasoning behind it. I was following along but had: <input type="checkbox" ${todo.completed ? 'checked' : ''}> ${todo.title} This became |
@jjenzz thanks for the feedback! - we'll probably be redoing a bunch of these tutorials for the upcoming choo v5 (yay, wayyy less things to learn!) and we should totes also mention the self-closing element gotcha. Thank you! :D |
I'd like to leave this issue here for readers to post feedback about the "Your First App" tutorial. I'm particularly interested in the things that you have to re-read 3 times because it didn't make sense the first two times, or concepts that may have been taken for granted.
Let me know if there's a preferred venue for this kind of feedback than a GH issue.
The text was updated successfully, but these errors were encountered: