-
Notifications
You must be signed in to change notification settings - Fork 5
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
Words Review Redesign #392
Conversation
+ Select words for the user to study based on their frequency counts in the learning language. This will help avoid situations where the user translates a lot of words in a single article and these would take up the entire pipeline. + Allow more user control by allowing them to edit words.
+ As words get deleted they need to notify their parent that the element shouldn't be rendered, otherwise there is a small space leftover. + Added a modal with an explanation to how words are selected.
+ Include Word in Exercises is now an editable field + Slight change to the delete button.
Considerations to prioritize words: 1. The user is reading an article and leaves without clicking on the review words -> We need to prioritize them when the component is unmounted. 2. The user goes to the review screen -> Do not perform it when the component is unmounted and only do it in the review screen. (Avoid the delay between API calls).
- Added explanation that the App remembers user altered words to prioritize studying.
+ Less emphasis on the delete button
+ Added constant for Max exercises per article.
Whenever a user selects a word to study, use the star functionality to remember that the user wants to study that word. This is done both in the Add/Minus and the Edit Form operations
Styling changes for Checkbox
Since the user may added bookmarks, update the number based on the wordsForExercises.
Added a title to word history
Remove console logging.
- Added a userPreference endpoint which will update the status with the user preferences. - Moved the prioritize bookmarks to the API, as it is updating the status of the bookmarks in the backend
- Added enum to read the inputs from user preferences
- Fixing the toggle switching if the InfoBox changes during edits.
✅ Deploy Preview for voluble-nougat-015dd1 ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
This is much more complicated than I've ever imagined :) solutions
I am tempted to let 1 happen. Let's see how people use this feature first before investing more in it. |
@mircealungu But it's not that hard to implement the behavior of nothing changing. Here's how I did it: This ensures if there is any bookmarks with user preference, then it's safe to assume the user has altered the original list - and that should cover most cases. This seeks to avoid the fact that I reduced the list to... say 2 words, and then I come back and have 10 again? |
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.
some discussion starters for you Tiago!
src/api/userWords.js
Outdated
@@ -1,4 +1,6 @@ | |||
import { Zeeguu_API } from "./classDef"; | |||
import { MAX_BOOKMARKS_PER_ARTICLE } from "../exercises/ExerciseConstants"; |
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.
max_bookmarks_to_study_from_article
?
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.
Changed!
src/components/ColumnWidth.sc.js
Outdated
const ContentOnRowModal = styled.div` | ||
display: flex; | ||
flex-direction: row; | ||
align-items: center; |
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.
the only difference here is a narrower margin. Could we take that as a parameter? I've never done that for styled components, but see no reason why this would not be possible.
Alternatively, if we want a narrower margin, this needs not be only in the modal, so maybe rename to something like CoRNarrowMargin?
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.
Actually, this is not needed anymore as it was part of styling for the Modal solution.
We can pass a parameter, but I think it would have to be overriden over style, not sure you can pass a value into a constant. Maybe you can make a function that return a styled.div?
src/exercises/ExerciseConstants.js
Outdated
@@ -1,2 +1,4 @@ | |||
export const MAX_EXERCISE_TO_DO_NOTIFICATION = 99; | |||
export const MAX_EXERCISE_IN_LEARNING_BOOKMARKS = 50; | |||
export const MAX_BOOKMARKS_PER_ARTICLE = 10; |
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.
MAX_BOOKMARKS_TO_STUDY_PER_ARTICLE
?
src/words/WordsToReview.js
Outdated
let newWordsExcludedExercises = []; | ||
let newWordExpressions = []; | ||
// Keep track how many words Zeeguu selected | ||
let newTotalWordsSelectedByZeeguu = 0; |
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.
newTotalWordsSelectedByZeeguu... Can the Zeeguu selected word list change?
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 was the discussion we had, after we make different preferences the number can decrease or increase.
src/words/WordsToReviewModal.js
Outdated
@@ -0,0 +1,195 @@ | |||
import Word from "./Word"; |
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.
maybe take this one out.
useState(false); | ||
|
||
useEffect(() => { | ||
let newWordsForExercises = []; |
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.
I still think I would prefer if the local temporary variables would be named with _
in front of them instead of new
. I'd be happy to join the search party (search-and-replace party) for all of them around the codebase :)
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.
I think if you believe it will make more sense in the long run we can do that, we should do it - but maybe let's take this as it is and then do a replace that's just renaming?
src/words/WordsToReview.js
Outdated
); | ||
|
||
const isNoWordsSelected = wordsForExercises.length === 0; | ||
const isZeeguuSelectWords = |
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.
i know it's a predicate, but sounds bad in English. can we rename to hasZeeguuSelectedWords
maybe?
src/words/WordsToReview.js
Outdated
totalWordsTranslated > MAX_BOOKMARKS_PER_ARTICLE && | ||
totalWordsEditedByUser === 0 && | ||
wordsForExercises.length > 0; | ||
const isUserEditedWords = |
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.
ditto
This has become quite a large change, but I will try to summarize the changes included.
This requires API changes: zeeguu/api#156
Main features:
user_preference
field which overrides, either positively or negatively thefit_for_study
status.fit_for_study
when we want to limit the schedule to simply 10 words per article.user_preference
Screencaps:
Review Word Screen
Review Word Screen (Editing)
Review Word Screen - InfoBox after user Edits
Review Word Screen - InfoBox if no words for exercises
New Edit Word Screen