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

Feature Show Reminder before answer (#3064) #3119

Merged
merged 3 commits into from
Apr 13, 2024

Conversation

Loudwig
Copy link
Contributor

@Loudwig Loudwig commented Apr 5, 2024

In the deck config you can choose to show a reminder instead of showing directly the answer at the end of the autoupdate time.

See here the discussion about this issue.

ftl/core/deck-config.ftl Outdated Show resolved Hide resolved
ftl/core/deck-config.ftl Outdated Show resolved Hide resolved
proto/anki/deck_config.proto Outdated Show resolved Hide resolved
@dae
Copy link
Member

dae commented Apr 6, 2024

Regarding your question about merging on the associated issue ticket, please merge the latest main changes into your PR branch. You may need to handle some merge conflicts, as there have been recent changes on the main branch to things like ts paths (eg ../components might become $lib/components, etc)

@Loudwig
Copy link
Contributor Author

Loudwig commented Apr 8, 2024

Ok i'll do theses changes, do i close this PR and do another one then ? @dae

@dae
Copy link
Member

dae commented Apr 8, 2024

You shouldn't need to close the PR - just (force)push your changes once you've made them.

@Loudwig
Copy link
Contributor Author

Loudwig commented Apr 9, 2024

I believe it's good now. I did ./ninja format and ./ninja check this time :). Should I try to rebase all of my commit into one or it's good @dae ? (Thanks for your patience).

Copy link
Member

@dae dae left a comment

Choose a reason for hiding this comment

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

Thank you - almost there!

Two remaining points:

  • let's put question action before answer action
  • please remove the core-repo change from this PR; that repo will be updated automatically when I next sync translations

deck-config-question-action-show-answer = Show Answer
deck-config-question-action-show-reminder = Show Reminder
deck-config-question-action = Question action
deck-config-question-action-tool-tip = The action to perform after the end of the time of the question. Either show answer and auto update or just show a reminder.
Copy link
Member

Choose a reason for hiding this comment

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

Let's drop the second sentence, as it simply repeats the dropdown options. Suggested wording:

The action to perform after the question is shown, and time has elapsed.

@dae
Copy link
Member

dae commented Apr 10, 2024

Should I try to rebase all of my commit into one

No need, that will happen when I squash-merge the PR.

@Loudwig
Copy link
Contributor Author

Loudwig commented Apr 10, 2024

while trying to remove core-repo and qt-repo from the PR i did some serious damage and i have strong difficulties going back. I figured that ftl/core-repo and ftl/qt-repo where updtated with the first merge commit. So i tried to revert it, then i updated the file of deck-config but it seems i did not do it properly...@dae

@dae
Copy link
Member

dae commented Apr 11, 2024

When you get stuck with git, sometimes the easiest solution is to copy the files you have changed, delete the branch you made, and make it again from the current main branch. Then you can copy the modified files in, and check nothing else was changed except the changes you made.

Added a option in the deck config that allow the user to choose in
Autoupdate mode between showing a reminder or revealing the card.
Also added my name to the contributors
@Loudwig Loudwig force-pushed the showReminderAfterQuestion branch from fb42da5 to b148615 Compare April 12, 2024 13:42
@Loudwig
Copy link
Contributor Author

Loudwig commented Apr 12, 2024

It is in deed faster than resolving the git issues that i had :)@dae.

ftl/core/deck-config.ftl Outdated Show resolved Hide resolved
@dae
Copy link
Member

dae commented Apr 13, 2024

Thank you!

@dae dae merged commit d8f2782 into ankitects:main Apr 13, 2024
1 check was pending
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants