zeeguu / web

Frontend for the zeeguu web application.
https://www.zeeguu.org
3 stars 5 forks source link

Make the like- and difficulty-box stay after interaction. #324

Closed Mlth closed 3 months ago

netlify[bot] commented 4 months ago

Deploy Preview for voluble-nougat-015dd1 ready!

Name Link
Latest commit 4c2e83bcf5ad320d6b82bca6646d20568aab20e9
Latest deploy log https://app.netlify.com/sites/voluble-nougat-015dd1/deploys/65f852c0d0631f00080e83d3
Deploy Preview https://deploy-preview-324--voluble-nougat-015dd1.netlify.app
Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Mlth commented 4 months ago

We have:

Here is a video:

https://github.com/zeeguu/web/assets/94927866/16af5edc-92f8-4e4e-a987-2accd185d3e0

oskaitu commented 3 months ago

We ended up not refactoring the icons very much. We only changed the name of the shouldBeMarked function. We tried to do some more refactorizations, but we are unsure whether they make the code better. These can be seen here, but have not been pushed:

OurSolutionCode OurSolutionCode2

Also, we moved the functions for updating the database when providing feedback for difficulty and likes. To ArticleReader. We did the same for the extension. We noticed that the OPEN_ARTICLE event was being logged every time the articleInfo updates. Should we create an issue for that? It looks like it is something that was done two weeks ago. This is where it happens (inside a useEffect): image

mircealungu commented 3 months ago

Thanks for exploring possibilities.

I think the most promising and reusable pattern would have been the one with DynamicIcon. Let's deploy the current version and keep thinking about a beautiful design please.

mircealungu commented 3 months ago

regarding your last observation, i think by modifying the articleInfo you've stepped on the toes of @tfnribeiro who implemented that dependency for a good reason.

@tfnribeiro : I think we might have to separate the behavior that needs to be called only the first time when the article is being opened (speech engine initialization, etc.) into a useEffect({ ... }, [])) from that that should be called when the articleInfo is modified (actually, no special behavior should be called in this case I think... or?).

However, I don't remember if you had an extra reason for making that useEffect depend on articleInfo. I guess maybe making sure that the articleInfo is loaded?

mircealungu commented 3 months ago

Ok, I looked a bit more attentively at the code.

I think the whole body of theuseEffect(..., [articleInfo]) can be inserted at line 201 in that file

image

The flight is about to take off, so I can't test it, but if one of you can, try that and if it works, update the corresponding PR. Tak!

oskaitu commented 3 months ago

Hi @mircealungu, I tested moving the body into the line that you specified. It sadly did not work as it didn't want to launch the extension and just kept going in a loop.

I've been trying some different things to get it to run without having a hook on articleInfo to no avail. It this point, I can't seem to wrap my head around what might be the problem. The useEffect you pointed to, should have all necessary information for the code to execute.

Would be nice with some experienced eyes on this :)

tfnribeiro commented 3 months ago

regarding your last observation, i think by modifying the articleInfo you've stepped on the toes of @tfnribeiro who implemented that dependency for a good reason.

@tfnribeiro : I think we might have to separate the behavior that needs to be called only the first time when the article is being opened (speech engine initialization, etc.) into a useEffect({ ... }, [])) from that that should be called when the articleInfo is modified (actually, no special behavior should be called in this case I think... or?).

However, I don't remember if you had an extra reason for making that useEffect depend on articleInfo. I guess maybe making sure that the articleInfo is loaded?

regarding your last observation, i think by modifying the articleInfo you've stepped on the toes of @tfnribeiro who implemented that dependency for a good reason.

@tfnribeiro : I think we might have to separate the behavior that needs to be called only the first time when the article is being opened (speech engine initialization, etc.) into a useEffect({ ... }, [])) from that that should be called when the articleInfo is modified (actually, no special behavior should be called in this case I think... or?).

However, I don't remember if you had an extra reason for making that useEffect depend on articleInfo. I guess maybe making sure that the articleInfo is loaded?

Okay, I was a bit lost, so I am guessing this is in the extension code? The reason why this has to be done was because I observed that when we would open a new article the component would not re-render with the new article information.

If the issue now is that we are re-running the open article, we can instead of tracking article info, track the ID before running that line. If the ID is the same as the one seen previously, then we just skip it.

I will have a look at the issue of the loader loop that @oskaitu saw.

tfnribeiro commented 3 months ago

I have taken a look, and if moving everything into the block that @mircealungu suggested seems to work OK!

My guess to the reason why the loop was stuck is that you need to update all mentions of articleInfo to artinfo. However, to avoid this situation where we have an infinite loading screen, I also propose adding a timeout of 10 seconds that just displays the ZeeguuError. I have made a PR with these suggestions: https://github.com/zeeguu/browser-extension/pull/52

oskaitu commented 3 months ago

Ah, yes, changing it into artinfo is definitely the thing I missed while trying to get it to work.

Great that it is solved!