Conversation
This comment was marked as outdated.
This comment was marked as outdated.
|
Sounds good @mxmason! Once all the docs updates are done, ping me again and I'll review this. |
chrisdavidmills
left a comment
chrisdavidmills
left a comment
There was a problem hiding this comment.
@mxmason I had a quick sweep through, and noticed some differences in the code base on here, versus the new one built up from your new tutorials. Shouldn't take a long time to fix up; just make sure the code bases are equivalent. Thanks!
|
|
||
| useEffect(() => { | ||
| if (tasks.length - prevTaskLength === -1) { | ||
| if (tasks.length < prevTaskLength) { |
There was a problem hiding this comment.
In my version of App.jsx:
- The
<ul>has aroleoflist. - The
editTaskfunction has full comments; the one in this repo has comments incomplete or missing
| @@ -1,4 +1,4 @@ | |||
| import React, { useState } from "react"; | |||
| import { useState } from "react"; | |||
There was a problem hiding this comment.
In my form component:
handleSubmit()is quite different; we don't have thetrim()check anymore.- The events use
eventrather thane
| @@ -1,4 +1,4 @@ | |||
| import React, { useEffect, useRef, useState } from "react"; | |||
| import { useEffect, useRef, useState } from "react"; | |||
There was a problem hiding this comment.
In my Todo file:
- As a general point, I noticed that in the new files generated from Vite, we are doing separate
export defaultstatements at the bottom of the file, whereas in the ones in this repo we haveexport defaultbefore the function name in the definition. - Again, our new Vite code doesn't include the
trim()check - The editing template
<input>no longer has aplaceholderin the new code.
| box-sizing: border-box; | ||
| } | ||
| *:focus { | ||
| *:focus-visible { |
There was a problem hiding this comment.
My index CSS file is significantly longer than the one in here, by about 50 lines.
There was a problem hiding this comment.
Heya, @chrisdavidmills! This is ready for another look :)
While I definitely had a few discrepancies to fix, some of what you're seeing confuses me! I don't know how you ended up with 50 extra lines in index.css, for example. The CSS in that file now is copied directly from the updated tutorial. I went back and referenced each branch to try to achieve parity with the tutorial – I've fixed the export patterns and the names of the event variables, for instance.
Other differences you're seeing might come from the fact that this code has been edited by people other than me. I never mention placeholder in the tutorial because of its myriad accessibility problems, so I wouldn't have noticed that someone snuck that in there if you hadn't said anything. Same for the .trim() functionality – we called that out as an exercise for the reader in the tutorial. I've added comments to hopefully prevent people from re-introducing trim() as a fix.
@mxmason Nice work! This looks good to me now. I hadn't reckoned on other people adding features to the repo not included in the tutorial! I didn't word it very well, but yeah, by comment was more supposed to be "why is this placeholder stuff here?" rather than "why haven't we included it?" I know that an a11y like your good self wouldn't include such things. The difference in CSS file size was due to my code editor automatically adding line breaks between each rule. My bad! |
This comment was marked as outdated.
This comment was marked as outdated.
4 participants
Description
As part of mdn/mdn#474, this PR migrates the app away from
create-react-appand ontoVite. This migration necessitates a few different kinds of changes. Among them:.jsfiles to.jsxReactimportspublic/index.htmlfor a root-levelindex.htmlWhile this PR has no dependencies, it's probably best to hold off until the tutorial in mdn/mdn#474 is fully updated.