-
Notifications
You must be signed in to change notification settings - Fork 521
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
Fixes part of #4195:Added dark mode support to QuestionPlayer and ExplorationPlayer #4540
Conversation
Here in this PR, I have not changed the cursor color as it was in PR #4382 since I figured out that the changes made in |
@bhaktideshmukh I'm not opposed to a separate issue tracking the specific problem so long as what we check in here doesn't break the UI/UX or crash the app. That being said, what exactly is the crash? Is there a debug doc that sheds light on it? Ideally we would keep all related dark mode changes together, but this might be a good case of separating this specific change if it's particularly challenging. |
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.
@bhaktideshmukh Suggested changes.
@BenHenning What I figured out while debugging was when I added |
@bhaktideshmukh did you figure out the root cause of the crash? Without understanding why the change causes the crash it's difficult to make decisions about alternative solutions. |
Yes @BenHenning you are correct without knowing the actual root cause we shouldn't proceed with the alternatives but I was unable to figure out why adding something externally in theme.xml lead to crash. |
@bhaktideshmukh thanks for the details. Per the SO answer, the android:textCursorDrawable is supposed to be set to a drawable, but I think you set it to a style. Only android:editTextStyle was set to a style in the answer. Is it possible the solution was implemented incorrectrly? |
@BenHenning Yes you were correct, the solution was incorrectly implemented and I apologize for that.
|
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.
LGTM, Thanks.
Explanation
This PR contains the changes of PR #4382 and the pending cases of this issue such as drag_drop_interaction, forward_arrow etc.
Mock link :--
Screenshots
Essential Checklist
For UI-specific PRs only
If your PR includes UI-related changes, then: