-
-
Notifications
You must be signed in to change notification settings - Fork 159
Update todo-list to demonstrate testing with ESM #790
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
Conversation
Your PR's title isn't in the expected format. Please check the expected title format, and update yours to match. Reason: Wrong number of parts separated by |s |
Your PR's title isn't in the expected format. Please check the expected title format, and update yours to match. Reason: Wrong number of parts separated by |s |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks great, thanks! I made a few suggestions here, and also a few in https://github.com/cjyuan/Module-Data-Groups/pull/1/files from when I was doing the exercise itself.
## `jsdom` | ||
|
||
[**`jsdom`**](https://github.com/jsdom/jsdom), a pure-JavaScript implementation of DOM for | ||
use with Node.js, **does not yet support** `<script type="module">` tags in HTML. | ||
|
||
Testing an ESM-based application with `jsdom` requires additional configuration and third-party tooling. No newline at end of file |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have we introduced JSDOM anywhere before in the course? If not, we probably want a whole "intro to JSDOM" before we write this, maybe in its own md file?
But I don't think we're even using it in this project, so we probably want to just remove this and introduce it if/when we introduce JSDOM?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All the Web app exercises in Sprint-3 folder (including the old Todo List app) include a Jest test script that uses JSDOM. I think that's when most trainees first encountered JSDOM. A few of them tried to understand how it works or tried to implement their apps to pass the tests, but most simply ignore the test script. (When I reviewed their code, I didn't use the test script to check their implementation).
Should we update one of the web app exercises to introduce testing with JSDOM? Last I checked it does not yet work with ESM unless we use some tool chain.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh wow, yeah, it looks like we wrote tests in a very "run these but don't edit or understand them" style, and we don't event mention them in the README. Let's maybe attach an order to the exercises (maybe starting with the quote generator?) and add an intro to the JSDOM tests in there?
Let's keep what you have here, and separately add a bit of JSDOM introduction! Thanks!
Also FYI I made CodeYourFuture/trainee-tracker#21 to stop validating NotCoursework PRs :) |
Co-authored-by: Daniel Wagner-Hall <[email protected]>
Co-authored-by: Daniel Wagner-Hall <[email protected]>
Co-authored-by: Daniel Wagner-Hall <[email protected]>
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Just that one suggestion about <span>
tags and I think we're good to merge!
I just edited #8 to mark it mandatory, I'll also rename the issues that sprint to make them be ordered usefully.
Co-authored-by: Daniel Wagner-Hall <[email protected]>
Changelist
Summary
This PR updates the ToDo List exercise with the following improvements:
Changes in Exercise
Before:
Now:
Related Issues in Curriculum
CodeYourFuture/curriculum#414
CodeYourFuture/curriculum#1289
CodeYourFuture/curriculum#1296
CodeYourFuture/curriculum#1448
Checklist
Who needs to know about this?
@illicitonion