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

User pronoun selection paths in the case conjugation display #218

Closed
2 tasks done
andrewtavis opened this issue Oct 4, 2022 · 10 comments
Closed
2 tasks done

User pronoun selection paths in the case conjugation display #218

andrewtavis opened this issue Oct 4, 2022 · 10 comments
Assignees
Labels
-next release- Included in the next release feature New feature or request help wanted Extra attention is needed

Comments

@andrewtavis
Copy link
Member

andrewtavis commented Oct 4, 2022

Terms

Description

With #210 being finished, one final issue is that if there is more than one pronoun in the field, that all options are inserted with the slashes, which is a problematic user flow. One way to solve this is that when a user presses something with more than one possible pronoun, they'll be taken to the 2x2 or a 3x1 conjugation display (to allow for spacing for the words) where they can select the annotation that fits the situation.

Contribution

This would be a good added feature for a future release after v2.0.0, but could be included to make sure that this feature is in a usable state from the start :)

@andrewtavis andrewtavis added feature New feature or request help wanted Extra attention is needed -priority- High priority labels Oct 4, 2022
@andrewtavis
Copy link
Member Author

andrewtavis commented Oct 4, 2022

Included in this would also be removing the pronoun ending marker on possessive pronoun displays (the letters in parentheses after the title). Instead the user will just be prompted with which ending to choose in the 2x2 conjugation menu after.

  • Check when done

@andrewtavis andrewtavis added the blocked Another issue is blocking label Oct 5, 2022
@andrewtavis
Copy link
Member Author

@andrewtavis andrewtavis removed the -priority- High priority label Oct 5, 2022
@andrewtavis andrewtavis changed the title Allow for selection of multiple pronouns in the case conjugation display Allow for selection of individual pronouns in the case conjugation display Oct 7, 2022
@andrewtavis andrewtavis changed the title Allow for selection of individual pronouns in the case conjugation display User journey pronoun selection in the case conjugation display Oct 7, 2022
@andrewtavis andrewtavis added the -next release- Included in the next release label Oct 7, 2022
@andrewtavis andrewtavis self-assigned this Oct 7, 2022
@andrewtavis
Copy link
Member Author

MVP for this is just going to do the journey for German and not Russian, which can be added at a later time :)

@andrewtavis
Copy link
Member Author

andrewtavis commented Oct 7, 2022

For this issue conjugateAlternateView needs to be made into an enum with the available layout options :)

  • Check when done

@andrewtavis andrewtavis removed the blocked Another issue is blocking label Oct 8, 2022
@andrewtavis andrewtavis changed the title User journey pronoun selection in the case conjugation display User pronoun selection paths in the case conjugation display Oct 8, 2022
@andrewtavis
Copy link
Member Author

Closed in c2bbf4a 🚀🙌

@andrewtavis
Copy link
Member Author

andrewtavis commented Oct 9, 2022

@SaurabhJamadagni, let me give you a run through of what happened in c2bbf4a. Sorry that I didn't do a PR on it, but then again I was just kind of changing things at will, and LOTS of things would have been needed to be looked at. Everything that was happening was within conjugation, which is still working fine, so I just wanted to save a potentially major headache for the review :)

What c2bbf4a did:

  • Renamed the pronoun case variables using the word "declension", which I remembered is what the conjugation of a pronoun is called
  • I renamed the variables that are used for conjugation and declension to "form", so now it's formFPS instead of conjFPS for what should be assigned to the top left of the 3x2 view
  • Removed individual label variables for the conjugation view and put them all in a dictionary formLabelsDict
    • What this allows us to do is clear them all by just iterating over the keys and setting them to "", which is necessary now that we have declension views switching to other declension views that have different dimensions (the labels of the old one would still show up)
  • returnDeclension is a massive function to cycle through declension views and trigger another loadKeys as well as return a desired pronoun if there's only one word in the field
    • This function is called from within returnConjugation if commandState == .selectCaseDeclension
  • formsDisplayDimensions is an enum that determines how many cells are in the conjugation/declension view, and replaces a boolean that said that the "alternate view" (2x2) for conjugation should be shown
  • There's also some cleanup and comments 😊

This was the last coding task for v2.0.0! I'm gonna chill and work on something else for the rest of the day, but before next weekend I'll be sure to make the new videos, do the data update, and then send it off!! 🚀 I'll also do some testing in the coming days just to make sure that everything's ok, which you're also welcome to do 😊

Hope you're having a nice weekend!

@andrewtavis
Copy link
Member Author

andrewtavis commented Oct 9, 2022

And @SaurabhJamadagni, just something that I'm realizing, it seems that getDefaultAutosuggestions is being called in cases when it shouldn't. I'm seeing them a lot more than I expect to, and specifically after I type a word with an annotation it's very regular that the first two defaults will be the ones that are shown after the annotation.

Example:

  • Type "Mit" and press space and you'll see the correct autosuggestions for it "dem" and "der" after the annotation
  • Delete once
  • Press space again and rather than "dem" and "der" you see "ich" and "die", the first two default autosuggestions for German

Should we open an issue for this? You're welcome to look into it if you'd like, as I expect it's just that some states are being crossed still :)

@andrewtavis
Copy link
Member Author

@SaurabhJamadagni, committing 1ff0c2a to try to fix the issue with getDefaultAutosuggestions. It seems to work at this point 😊

What that changed:

  • I got rid of secondaryPastStringOnDelete as to me it seemed like we could more easily get the word we want by selecting the second to last word in the proxy when separating by spaces
  • The function clearCommandBar seemed to be a function that was from the past UI, so I removed it (had been considering this for a bit)
  • Added some lines to remove the annotation even if the user presses space (had been buggy as the keyboard wasn't loading again, so it'd just stay)
  • Made it so that deleting back to a space will autosuggest (if a user had multiple spaces and deleted one, then it'd try to complete and just present the first three words alphabetically)
  • Other minor changes :)

Coding wise it all seems ok to me at this point! Let me know what you think 😊

@SaurabhJamadagni
Copy link
Collaborator

Sorry that I didn't do a PR on it, but then again I was just kind of changing things at will, and LOTS of things would have been needed to be looked at.

No worries @andrewtavis!

This was the last coding task for v2.0.0! I'm gonna chill and work on something else for the rest of the day, but before next weekend I'll be sure to make the new videos, do the data update, and then send it off!! 🚀 I'll also do some testing in the coming days just to make sure that everything's ok, which you're also welcome to do 😊

Sounds awesome! Yup I'll test a little bit too! Have a chill rest of the weekend 😊

@SaurabhJamadagni
Copy link
Collaborator

I got rid of secondaryPastStringOnDelete as to me it seemed like we could more easily get the word we want by selecting the second to last word in the proxy when separating by spaces

Yes that's perfect. I planned to do that myself anyway. The seperating words by the space seperator into an array is a much better method.

Coding wise it all seems ok to me at this point! Let me know what you think 😊

All looks good @andrewtavis! Testing will be the key now I think. Thanks for fixing this one! Looking forward to v2.0.0!! 🚀 😊

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
-next release- Included in the next release feature New feature or request help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

2 participants