Add lighten style #1233
Add lighten style #1233
Conversation
|
Hmm, I think maybe a better way to do this (both from a UI perspective, and logic perspective), is to make the weight of the instance/name a slider with several discrete values. We can use the Slider widget to build out a slider, and have three presets (light, normal, bold) which controls a single setting (e.g., userFontWeight). We can then store the FontWeight parameter as the setting and use that rather than have two separate settings for bold/lighten. A upside of doing this is that we can add more settings to make it more fine-grained if we want to! Thoughts? |
|
Yeah, that's a good call! I was putting that off because I knew it would be a lot of work, but it's for the best long term! I've abstracted both thickness and color into new settings which should be more generic going forward. The former now allows the three values that you suggested. The latter allows Let me know if you think this looks good! And again let me know if you think the new default (only lightening the instance name for both users and communities) is good going forward. qemu-system-x86_64_LLJjI491JN.mp4 |
|
The latest demo looks really good! Thank you for making the changes I described earlier 😅. I haven't taken a look at the code yet but I'll try to review that as soon as I have time! |
lib/utils/preferences.dart
Outdated
| await prefs.setBool('user_fullname_colorize_user_name', legacyCommentUseColorizedUsername); | ||
| } | ||
|
|
||
| // Migrate legacy user/community styles |
There was a problem hiding this comment.
Do we need to migrate these settings? I believe #1227 was never released in a nightly build so we might be able to get away with not having to do any migrations.
There was a problem hiding this comment.
Yeah good point! I was probably overzealous with the migrations, but realistically no one besides us has even had the chance to run this. 😆
lib/utils/preferences.dart
Outdated
| @@ -24,6 +25,64 @@ Future<void> performSharedPreferencesMigration() async { | |||
| bool? legacyCommentUseColorizedUsername = prefs.getBool(LocalSettings.commentUseColorizedUsername.name); | |||
| if (legacyCommentUseColorizedUsername != null) { | |||
| await prefs.remove(LocalSettings.commentUseColorizedUsername.name); | |||
| await prefs.setBool(LocalSettings.userFullNameColorizeUserName.name, legacyCommentUseColorizedUsername); | |||
| await prefs.setBool('user_fullname_colorize_user_name', legacyCommentUseColorizedUsername); | |||
There was a problem hiding this comment.
We can replace this with await prefs.setString(LocalSettings.userFullNameUserNameColor.name, const NameColor.fromString(color: NameColor.themePrimary).color); if we're not worried about doing the migrations below
There was a problem hiding this comment.
Agreed! I think we can set it only if the old value was true. I also think we can use the string value directly without having to construct the NameColor. Let me know what you think of the latest.
|
If I may, I have a suggestion for you: I think that the settings related to how user/community looks would fit better somewhere under "Appearance" in the settings. Especially now that there are 4 of them. Maybe in "Appearance -> Theming", where the font sizes are? Thanks for your work! |
Good idea! Moved! qemu-system-x86_64_tVxFcUw78t.mp4 |
Pull Request Description
After daily driving #1227 for a while, I'm a bit concerned that the username doesn't stand out enough in comments. However, I'm still not sure I want to make it bold by default since that has other unintended side effects in the app. Therefore, there PR adds a new style, which lightens the instance name. Previously the instance name would automatically get lightened when bolding the username (and vice versa) but now the option to lighten is separate. I also put it on by default. The nice thing about this approach is that it doesn't affect usernames where the instance isn't shown, which is the default in several places.
Let me know if you think it looks good and is a good default going forward!
Issue Being Fixed
Issue Number: N/A
Screenshots / Recordings
qemu-system-x86_64_bOJmVbuUeX.mp4
Checklist
semanticLabels where applicable for accessibility?