Articles can be "completed" #1

potpov · 2021-10-24T21:43:59Z

Each time all the words for a given article are guessed, the article is marked as completed and the user interface gets updated. Once all the articles from the level are completed, the user is moved to the next level and loads harder articles (still static).

The idea: each user has a level and on each level, we fetch a list of articles, progressively more complex and with a new vocabulary. Hence the user is supposed to find and learn a bunch of new words per level.

animation shows up on single words now - match left counter added.

tom95

Very nice changes, in particular also thanks for updating for the changes in MaterialUI!

I added some feedback on the stylistic/idiomatic level. I also believe the project was originally formatted using an automatic pretty-printer but I could be wrong. If not, it may make sense to configure prettier.js for the project and run it on the whole codebase once, this may be easier than hunting for small whitespace differences (some of which I had made comments for, so you can ignore these :))

(Note that I didn't get around to running the code yet)

src/App.js

tom95 · 2021-10-26T05:45:05Z

src/ArticleList.js

    const [open, setOpen] = React.useState(false);
    const [content, setContent] = React.useState('');
+    const [notification, setNotification] = React.useState('');
+    const [articles, setArticles] = React.useState(fetch_articles());
+    const [level, setLevel] = React.useState(fetch_user_level());


I know the original code was not consistent either but maybe you could make it consistent in one swoop and remove the React. prefix for the original two lines as well, so that it's the same as useParams and useHistory below?

tom95 · 2021-10-26T05:45:33Z

src/ArticleList.js

+    useEffect(() => {
+        // TODO: check from state if all articles are completed -> level up!
+        // use:
+        if (articles.filter((article) => !article.completed).length == 0) {


Consider using articles.every :)

nice, thanks! there is still a bit of rust on JS to take off :D

tom95 · 2021-10-26T05:47:50Z

src/ArticleList.js

+    const markAsComplete = (article_id) => {
+        let updated_articles = articles.map(
+            (article) => {
+                return((article.id == article_id && !article.completed) ? {...article, 'completed': true}: article


some minor style comments, I would have probably formulated it as:

articles.map((article) => article.id === articleId ? {...article, completed: true} : article)

Note camelCase instead of snake_case, the triple ===l, and changed whitespace

tom95 · 2021-10-26T05:49:29Z

src/ArticleList.js

+                )
+            }
+        )
+        setArticles(updated_articles)


ah, take the line from above and then insert it here:

setArticles((articles) => articles.map(...))

if you update a value in React, the recommended pattern is to use these update functions instead of reading the value first. This prevents using an out-of-date articles value if multiple "set"s happened before the next rebuild.

Yes, you are right.
I initially made the all-articles-completed check on this function; as setState methods won't reflect state changes immediately I was using the tmp variable as a workaround. I figured there's the useEffect function for this purpose but forgot to edit the line :)

Also, react-dom inherently treats URL params as strings; will force casting on the next commit to add the strict equality here.

tom95 · 2021-10-26T05:49:47Z

src/ArticleList.js

+
+    }
+
+    function get_article(article_id){


consider changing to camelCase as well :)

tom95 · 2021-10-26T05:52:24Z

src/articles/Article.js

+    Setmatch_left(match_left - 1)
+  }
+
+  const reactStringReplace = require('react-string-replace');


oh wow this worked? I didn't know you could that tbh :D

I believe the more idiomatic approach (correct me if I'm wrong for this library) would be to move it to the very top of the file and use esmodule-style imports, along the lines of import { reactStringReplace } from 'react-string-replace; or similar.

corinnaj · 2021-10-30T11:39:38Z

src/ArticleList.js

+            'id': 0,
+            'completed': false,
+            'title': 'Apple Pie',
+            'content': 'German Apple Cake is a traditional German dessert that is so easy to make even if you aren’t totally kitchen confident! With a simple batter that rises up and bakes around the apples this easy apple coffee cake is the perfect everyday dessert that tastes best with a dollop of whipped cream on top.',


I think you replaced the content with the preview? At least when I open an article, I also only see this short text, rather than the steps and methods, as in ApplePie.js.

yup. can fix it easily but I am actually craving for a model and a migration with all articles to be run on init so we can clean up this file.

Yeah, in the long run that would definitely be more convenient.

corinnaj · 2021-10-30T11:41:01Z

src/ArticleList.js

+            'id': 2,
+            'completed': false,
+            'title': 'Capitain Bluebear',
+            'content': 'The 13¹⁄₂ Lives of Captain Bluebear is a 1999 fantasy novel by German writer and cartoonist Walter Moers which details the numerous lives of a human-sized bear with blue fur.apple',


Why is there "apple" at the end of almost every article? Might be something we had for the demo, but we probably should remove it now :D

Just for the demo right, you need at least one word to guess on each article or you won't be able to complete it and level up (laziness got real here and I just added apples everywhere sorry) :D

corinnaj · 2021-10-30T11:44:54Z

src/PartialTranslationParagraph.js


  const handleSubmit = (answer) => {
    if (confirmWord(german, answer)) {
      setSuccess(true);
      setOpened(false);
+      onComplete();


When is this being triggered? I got all words to "white" in the apple pie article, but it wasn't marked as completed.

Oh, I think the problem was that I had to click on every occurrence of the world "apple" again, even though it was already white.

exactly! You have to guess all the words to complete an article (: just like an exercise. but the interface is confusing now.
Since the beginning, I thought we have to change the confetti effect to be only on the occurrence of the word you guess. it is a bit counterintuitive atm (now even more with this update) - will look at it

corinnaj · 2021-10-30T11:45:17Z

package.json

-  "proxy": "http://localhost:8000"
+  "proxy": "http://localhost:8000",
+  "devDependencies": {
+    "prettier": "2.4.1"


strict comparison on URL, removed commented routes, camel case refactoring

- Partial translation component is now loaded correctly into the DOM even when an article contains html tags - Old articles body restored - Minor warning from Material UI fixed

potpov · 2022-01-03T21:20:44Z

Alright,
I think I solved pretty much all the issues you highlighted for this first update. Let me know if I missed something.
Adding back the content of the articles arose some bug that took me quite a while to solve. Static articles fetch is still icky to see but it's just a mock-up I plan to remove next with some real API.

potpov added 7 commits October 24, 2021 23:06

Article moved below ArticleList

1893769

reverse mapping simplified with reactStringReplace

fd1dcae

React forwardRef bug fixed

033a0ed

animation shows up on single words now - match left counter added.

Introducing completed articles

630983a

articles progress bar

5d46ce2

level up demo

99e91bd

restore lock.json

6c5d0ca

tom95 requested changes Oct 26, 2021

View reviewed changes

corinnaj requested changes Oct 30, 2021

View reviewed changes

refactoring

0c68cc5

strict comparison on URL, removed commented routes, camel case refactoring

potpov force-pushed the master branch from 4f1aa9b to 0c68cc5 Compare October 30, 2021 23:12

word replacement now handles html in articles

3f83a49

- Partial translation component is now loaded correctly into the DOM even when an article contains html tags - Old articles body restored - Minor warning from Material UI fixed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Articles can be "completed" #1

Articles can be "completed" #1

potpov commented Oct 24, 2021

tom95 left a comment

tom95 Oct 26, 2021

tom95 Oct 26, 2021

potpov Oct 26, 2021 •

edited

Loading

tom95 Oct 26, 2021

tom95 Oct 26, 2021

potpov Oct 27, 2021

tom95 Oct 26, 2021

tom95 Oct 26, 2021

corinnaj Oct 30, 2021

potpov Oct 30, 2021

corinnaj Oct 30, 2021

corinnaj Oct 30, 2021

potpov Oct 30, 2021

corinnaj Oct 30, 2021

corinnaj Oct 30, 2021

potpov Oct 30, 2021

corinnaj Oct 30, 2021

potpov commented Jan 3, 2022

Articles can be "completed" #1

Are you sure you want to change the base?

Articles can be "completed" #1

Conversation

potpov commented Oct 24, 2021

tom95 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

potpov Oct 26, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

potpov commented Jan 3, 2022

potpov Oct 26, 2021 •

edited

Loading