search

anki-geo / ultimate-geography

Geography flashcard deck for Anki
https://ankiweb.net/shared/info/2109889812
Other
823 stars 81 forks source link

Anki v24.06+ back card side automatic swap fix #651

Closed helitopia closed 3 months ago

helitopia commented 3 months ago
helitopia commented 3 months ago

Force pushed to resolve merge conflict as forgot to sync master branch in fork with the upstream.

helitopia commented 3 months ago

Maybe a little comment to explain that code is for backwards compatibility with Anki <= x.y?

I hesitated when writing commit message, as it is already mentioned in the discussion:

I tried to fix it by setting both properties (for it to work on both pre-24.06 and post- versions): new KeyboardEvent("keypress", {code: "Enter", key: "Enter"}). It worked, so I will open PR to resolve the issue (@aplaice @axelboc or should I open issue first?).

And since I am linking the discussion thread, thought that it would be redundant.

axelboc commented 3 months ago

I'm all for writing readable code and limiting the number of comments, but this is a case where a very short comment can be super helpful in the long term.

It can be tedious to find discussions linked to a piece of code: one has to "blame" the specific line to find which commit last modified it then go to the corresponding PR and then to the related discussion. It becomes a lot more difficult when that piece of code gets moved to a different file or gets changed multiple times...

In 5 years' time, if it stops working or if someone stumbles upon this code and thinks that code is redundant and wants to remove it, it'd be good to inform them right away that they should first check whether the version of Anki it targets is still in use or not.

aplaice commented 3 months ago

Thanks very much for diagnosing and fixing this!

~This is nitpicking, but I'd also go with a comment in the code. (My preference here: comment > git commit message > link.) (Having the info available in git blame is nice, but IMO this is one of these "weird" things that ideally should just be commented in the code.)~ (Edit: @axelboc wrote the same, better phrased, 2 minutes before me :D)

helitopia commented 3 months ago

It is not a problem, completely agree that a shortcut for explanation would be helpful. I guess I can then move IIFE reasoning from wiki into code as well.

helitopia commented 3 months ago

Separated into two commits, one for the card swap and the other - IIFE docs.

Not sure why, but I am somewhat triggered by regular comments in code. Probably due to having prior experience with the project that compensated for shitty code with comments everywhere :). Hence, refactored into separate function with proper JSDoc comment above it.

Tell me what you think about it.