Web applications part 1 (Browser.sandbox)#730
Conversation
Still need to create introduction.mdonce about.md has been through review
Its numbers and not letters, but not too bad
Now it handles the case when the name of the primary file (src/Main.elm) doesn't match the solution name. This is a good convention, but doesn't work for the web applications concept where the primary file must be called Main.elm to work with elm-test and elm-make and similar
Not sure it is going to work though
This reverts commit 5532072.
I don't think it's necessary, raw HTML looks good enough for this purpose. Plus, it would probably require to make assumptions about how the HTML is structured and stf. |
Co-authored-by: Jie <jie.gillet@gmail.com>
I couldn't reproduce the build error locally, or find a way to commit the change that the build script needed.
|
HI @jiegillet, I've responded to all your points now, can you take another look. |
|
I'll check again this weekend :) |
jiegillet
left a comment
jiegillet
left a comment
There was a problem hiding this comment.
Alright!
I'm good with the concept about.md, and the exercise. Which didn't prevent me from adding 6000 suggestions 🙇🙇🙇
I'm still not sold on elm-watch though, and I would like to have @mpizenberg's opinion (tagged on the relevant comment) if that's OK.
| } | ||
| } | ||
| } | ||
| } No newline at end of file |
There was a problem hiding this comment.
This elm.json is not identical to template/elm.json because of the missing newline, but it's not flagged because you swapped cmp for diff -w in the sanity check. Was that intentional?
I think it's best to keep the files exactly identical, it's just less noisy that way.
Also, in general I think it's good practice to always end file on a newline, but that's another thing.
There was a problem hiding this comment.
This is intentional, I wasn't able to make them the same. using cp didn't work, and my editor removes the new line if I try and do it manually. Maybe there is some crlf stuff going on as well. Anyway, I spent some time trying to get these the same and failed, and its irrelevant, so I think its probably better this way, as the same situation can happen to somebody else and its just wasting time for no benefit.
Co-authored-by: Jie <jie.gillet@gmail.com>
|
Hi @jiegillet, I've made those changes, apart from the one we are waiting on Matthieu for, one that wouldn't work (Debug.todo), and the whitespace issue for elm.json (where the juice isn't worth the squeeze). |
|
Hi @jiegillet , I don't think Matthieu is going to look at this, so we will have to decide ourselves. I think elm-watch is easier to use, and better (as it has hot reloading) and more idiomatic. Being as students will already be running the code locally in this case they will almost certainly have npm installed, and npx is bundled with npm, so I don't think we are really pushing any extra dependencies on students. |
There was a problem hiding this comment.
Ok, let's do it :)
I couldn't help myself, I made all the elm.json templates identical, I hope you don't mind. EDIT: welp, I am stumped, I didn't manage to do it, you were right they are identical locally, byte by byte, but exercises/concept/paulas-palindromes/elm.json is deemed to be different on CI. No clue why, I gave up.
Sorry for being so nitpicky, you really did a great job for this exercise.
All that's left in the intro file.
|
I think the file must be different in the repo / on ci, but there is some nuance in the way git handles / stores the whitespace / line endings. I was also surprised that it wasn't fixable! No worries about the nit picks. You make a lot of great points. I don't 100% agree with all of the comments, but I generally don't disagree enough to worry about it :) |
3 participants
Fixes #639
Replaces the previous draft PR #723 where there is a lot of discussion about how and what to do.
I'll do the introduction.mds from about once people are happy with it.
We could also add a css file so that it looks a bit nicer if you run it locally, but not sure if its worth it.