Fix the Bookmark Context not updating when user updates in exercises #519
Conversation
- The interactive text object needs to be re-created as this is what contains the change, so a flag was created that gets called when the bookmark is changed.
✅ Deploy Preview for voluble-nougat-015dd1 ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
| @@ -252,6 +253,7 @@ export default function NextNavigation({ | |||
| reload={reload} | |||
| setReload={setReload} | |||
| notifyDelete={() => setIsDeleted(true)} | |||
| notifyWordChange={() => isBookmarkChanged()} | |||
There was a problem hiding this comment.
Ehi Tiago, could this be a sign that we should move the edit button away from the next navigation? Or rename NextNavigation to something else? Currently it feels wrong to pass the notifyWordChange to the NextNavigation which passes it on further to the edit box...
I think the right direction would be to move both the speech button and the edit button. We've already talked about the speech into the main exercise as underline. If we also move the edit box, we could have a design like this:
We can almost skip passing the exerciseBookmark down to the NextNavigation too... because as far as I can tell, it's mainly used for the Speech and for the EditBookmark dialog.
In any case, I would say, the first step could be moving the Edit bookmark button, so the button lives in the same component as the bookmarks that it edits.
Do you see what I mean?
P.S. Surely, we could also, just as well not do it now, but rather open an issue instead.
There was a problem hiding this comment.
So the NextNavigation originally was just to create a component to avoid repeating a lot of the same formatting for each of the exercises. I think the main reason was because some exercises had speech and other didn't have the speech bubble (Match vs Others). Eventually that was also where Merle has implemented the celebration modal logic/message of learning a word.
This means that the component which essentially was meant to just place these icons in a bottom row in a standard way has gotten much more complex than it should.
I agree with having the speech / edit in the text, and removing the pronounce buttons from the end, and that should make it easier to unpack the component.
Now, I think we should still keep this idea that the exercise renders what it has to render, but the Next / Congratulations / Auto-Pronounce these are all exercise independent and they just just be placed bellow the current exercise. This would mean that this logic should not be part of the Specific Exercise implementation, but rather controls to the ExerciseSession.
I think to do this well it will take a bit of time, there is a lot of things that would have to be moved, but it would be beneficial to make sense of the code.
Essentially, this:
Would be part of a component in the Exercise code, rather than in each exercise itself. We still have the issue of having to test if it's the Match, but maybe we can just have a generic "MultipleWordsProgressed" logic for the rendering of the congratulations box.
There was a problem hiding this comment.
Let's merge this for now because it's a bug fix. Then we'll do the refactoring at a later point. I've created an issue with this conversation: #535.
Notes