Skip to content
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

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

Merged
merged 3 commits into from
Aug 24, 2024

Conversation

helitopia
Copy link
Collaborator

@helitopia
Copy link
Collaborator Author

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

Copy link
Collaborator

@axelboc axelboc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@helitopia
Copy link
Collaborator Author

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
Copy link
Collaborator

axelboc commented Aug 24, 2024

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
Copy link
Collaborator

aplaice commented Aug 24, 2024

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
Copy link
Collaborator Author

helitopia commented Aug 24, 2024

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.

- Specifically the intention of the introduced documentation is to keep a notice of the reasoning of IIFE usage
@helitopia
Copy link
Collaborator Author

helitopia commented Aug 24, 2024

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.

Copy link
Collaborator

@axelboc axelboc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot!

src/media/experimental_assets/_ug-interactive_map_init.js Outdated Show resolved Hide resolved
@axelboc axelboc merged commit d33eb54 into anki-geo:master Aug 24, 2024
1 check passed
@axelboc axelboc added the structure Templates, tags, generated decks, etc. label Aug 24, 2024
@axelboc axelboc added this to the v5.3 milestone Aug 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
structure Templates, tags, generated decks, etc.
Development

Successfully merging this pull request may close these issues.

3 participants