refactor: simplify settings string summaries#1366
refactor: simplify settings string summaries#1366krvstek wants to merge 11 commits intoMorpheApp:devfrom
Conversation
|
@kitadai31, have you started working on a PR or not? |
krvstek
force-pushed
the
refactor
branch
3 times, most recently
from
408e4dd to
869ebaf
Compare
|
To test this PR, we can compile it in GitHub actions and add it to the manager as a remote source (manager supports PR links as a remote source, need to add Github PAT in settings) or as a local file. |
I started working on it a little bit, but soon I got busy with other tasks, so I haven't touched it for a month. The new strings look good. However, I felt that many of the settings lacked sufficient description. For reference, I tried pushing some code that I had left untouched for a month. |
krvstek
force-pushed
the
refactor
branch
3 times, most recently
from
f144cf0 to
e86b00f
Compare
I've tested the changes locally and everything works fine, haven't noticed any bugs so far. I'll do a final review today to make sure my changes align well with the current scope of the PR, do some minor tweaks if needed, and I will mark it as ready today or tomorrow. |
|
At first glance, it's LGTM, but since I'm not a native English speaker, I can't help with editing the text. Also, some menu items don't have summaries right now. Screenshots
We also need to test the search, I remember during its development there was a problem with text highlighting after changing the text in the summary when changing the settings. Maybe there are places of potential optimization somewhere after these changes. |
That was completely intentional, I felt that many toggles (like 'Hide video ads' or 'Hide fullscreen ads') are entirely self-explanatory, so adding a summary just cluttered the UI. However, as I mentioned before, UX is highly subjective. If you feel any specific toggles absolutely need their descriptions back for clarity, just let me know and I can add them. |
|
Oh, of course, I looked at the YouTube settings, there are also such menu items without a summary. Well yes, it's subjective, personally I don't care, since I almost never go into the settings)). Only when I need to test something. |
|
I'll try to test everything by this evening. I'm also going to do a final review of the strings to see if anything needs further simplification, or if any specific toggle actually requires a summary to avoid confusion. Once I'm done and everything works fine, I'll mark the PR as ready. |
| <string name="morphe_hide_featured_videos_section_title">Hide Featured videos section</string> | ||
| <string name="morphe_hide_gaming_section_title">Hide Gaming section</string> | ||
| <string name="morphe_hide_how_this_was_made_section_title">Hide \'How this content was made\' section</string> | ||
| <string name="morphe_hide_how_this_was_made_section_title">Hide content disclosure</string> |
There was a problem hiding this comment.
This change feels against the purpose of the PR. The title is enough isn't it?
There was a problem hiding this comment.
This change feels against the purpose of the PR. The title is enough isn't it?
I tested the long title (Hide 'How this content was made' section) and the text gets truncated in the UI right around the word 'section'. Using a shorter title (Hide content disclosure) and keeping the exact phrase in the summary is the only way to make it fully readable on mobile screens without cutting off.
4 participants
Description
This PR cleans up how we handle settings descriptions across all apps. Instead of maintaining separate
_onand_offstrings for every single toggle, we're moving to a single, unified_summarythat just describes what the feature does.Closes #1097.
Additional context
This cuts down the string payload and reduces the translation work almost in half.
Gonna put this PR into draft first, because it's a massive UX change and somewhat subjective, so I want to get your thoughts first. Let me know if you think I removed a summary that is actually needed.
Test results