Skip to content

Conversation

angrezichatterbox
Copy link
Member

Contributor checklist


Description

This PR adds conjugate support for Swedish and Russian language and fixes the bugs present in the German conjugate mode that caused it to not produce the right output for Perfect tenses.

Related issue

Copy link

Thank you for the pull request! ❤️

The Scribe-Android team will do our best to address your contribution as soon as we can. If you're not already a member of our public Matrix community, please consider joining! We'd suggest that you use the Element client as well as Element X for a mobile app, and definitely join the General and Android rooms once you're in. Also consider attending our bi-weekly Saturday dev syncs. It'd be great to meet you 😊

Copy link

github-actions bot commented Jul 20, 2025

Maintainer Checklist

The following is a checklist for maintainers to make sure this process goes as well as possible. Feel free to address the points below yourself in further commits if you realize that actions are needed :)

  • The linting and formatting workflows within the PR checks do not indicate new errors in the files changed

  • The CHANGELOG has been updated with a description of the changes for the upcoming release and the corresponding issue (if necessary)

@angrezichatterbox angrezichatterbox force-pushed the feat/conjugate-for-swedish-russian branch from a337673 to 5aa327e Compare July 20, 2025 09:21
@angrezichatterbox
Copy link
Member Author

I have made the changes. It is ready for review.

@andrewtavis
Copy link
Member

A couple of quick comments here, @angrezichatterbox :)

  • The verb isn't being read in for Swedish if it's capitalized
  • We have a slight overflow of the label of the bottom right 2x2 cell (see screenshot below)
Screenshot_20250724_224705

I think we should be good after these changes come in!

@angrezichatterbox
Copy link
Member Author

I will add these changes in a while.

@angrezichatterbox
Copy link
Member Author

 "1": {
            "title": "Aktiv",
            "conjugationTypes": {
                "1": {
                    "title": "Aktiv",
                    "conjugationForms": {
                        "infinitiv": "activeInfinitive",
                        "presens": "activePresent",
                        "preteritum": "activePreterite",
                        "supinum": "activeSupine"
                    }
                }
            }
        },
        "2": {
            "title": "Passiv",
            "conjugationTypes": {
                "1": {
                    "title": "Passiv",
                    "conjugationForms": {
                        "infinitiv": "passiveInfinitive",
                        "presens": "passivePresent",
                        "preteritum": "passivePreterite",
                        "supinum": "passiveSupine"
                    }
                }
            }
        }

Could this modification be temporary made to the contract ?

@angrezichatterbox
Copy link
Member Author

In that case this is ready for review.

@angrezichatterbox
Copy link
Member Author

I would have to make that change as the currently the entire top small label is being singly set using the a single logic rather than being key wise.

@andrewtavis
Copy link
Member

andrewtavis commented Aug 10, 2025

I'm ok with the change to the contract, @angrezichatterbox, but it looks like we still have UI issues regardless, and maybe something from the recent commits is also not interacting well with the height of the conjugation buttons:

Screenshot_20250810_145727

Could we get the labels over the buttons and have the combined height of the buttons span the height of the conjugation display?

@angrezichatterbox
Copy link
Member Author

I will have it fixed right away.

@andrewtavis
Copy link
Member

Nice! Looking forward to bringing this in, @angrezichatterbox! 😊

@angrezichatterbox
Copy link
Member Author

Can I Know which Device is it being tested on ?

@andrewtavis
Copy link
Member

Pixel 9 API 35 :)

@angrezichatterbox
Copy link
Member Author

It is ready for review :)

@angrezichatterbox
Copy link
Member Author

I would bring in @DeleMike suggestion in this PR.

@andrewtavis
Copy link
Member

The one about the text sizes of the keys? Feel free to add that in!

@andrewtavis andrewtavis mentioned this pull request Aug 10, 2025
2 tasks
@DeleMike
Copy link
Collaborator

DeleMike commented Aug 10, 2025

I would bring in @DeleMike suggestion in this PR.

Hi @angrezichatterbox I've looked at the conjugation section and all looks good to me! or is there a specific section of conjugation you want me to take a look at

See a video recording:

Screenrecorder-2025-08-10-18-04-16-222.mp4

@angrezichatterbox
Copy link
Member Author

This is ready for review then.

Thanks @DeleMike

Copy link
Member

@andrewtavis andrewtavis left a comment

Choose a reason for hiding this comment

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

This is looking good now, @angrezichatterbox :) The buttons and labels might still need a bit of work before we release as they're still not exactly aligned at the bottom and the labels go over the edge the slightest bit, but nothing to worry about right now 😊 Thanks for your continued efforts! Let's get English done and then we can maybe create help wanted issues to finalize the minor display things there or even check it out during a sync :)

@andrewtavis andrewtavis merged commit 684beda into scribe-org:main Aug 11, 2025
6 checks passed
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.

3 participants