-
Notifications
You must be signed in to change notification settings - Fork 526
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
Fix #4446: Add placeholders for text based interactions #4503
Conversation
@BenHenning PTAL |
@@ -179,6 +179,42 @@ class MathExpressionInteractionsViewModel private constructor( | |||
} | |||
} | |||
|
|||
private fun foldPlaceholderText(placeholder: CharSequence): String { | |||
val numberOfLettersInOneLine = 50 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doesn't this depend on display properties, such as text scaling, display density, and portrait/landscape orientation?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. My updated solution handles all this.
@JishnuGoyal I'm wondering this is solving an unrelated issue. Wasn't the original issue leading to #4446 that the placeholders were not showing up for some lessons? |
@BenHenning PTAL |
Unassigning @JishnuGoyal since a re-review was requested. @JishnuGoyal, please make sure you have addressed all review comments. Thanks! |
@@ -24,7 +24,7 @@ | |||
android:id="@+id/math_expression_input_interaction_view" | |||
style="@style/InputInteractionEditText" | |||
app:placeholder="@{viewModel.hintText}" | |||
android:inputType="text" | |||
android:inputType="textMultiLine" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So I think the new issue is that this results in the default ime action in the keyboard now being to insert a line rather than submitting an answer, which I think is problematic. I'll look into what EditText does in textMultiLine mode to see if we can somehow get it to support multi-line hints without actually being a multi-line box.
I believe this is obsolete now that #4506 was merged. |
Explanation
Additional note: At the time of PR creation it was observed that the placeholder texts are displayed everywhere and match with the ones on the website.
This PR fixes #4446 by adding a function that folds the placeholders texts (while also making sure that the lines are folded at a word break). These placeholder texts are "TextView hints" -- which can't fold the hint text as an out of the box feature in android.
In the function created called "foldPlaceholderText", there is an adjustable variable called
numberOfLettersInOneLine
. On tweaking this, the function can handle changing lines after the adjusted number of letters, or after few more letters depending on where is the immediate next word break.Essential Checklist
For UI-specific PRs only
If your PR includes UI-related changes, then:
Before:
Portrait
Landscape
After:
Portrait
Landscape