feat(android): design improvement for settings, add dark mode and tool menu#15827
feat(android): design improvement for settings, add dark mode and tool menu#15827Mengchou97 wants to merge 39 commits intokeymanapp:masterfrom
Conversation
…into dark-mode-on-side-panel
User Test ResultsTest specification and instructions User tests are not required |
|
This pull request is from an external repo and will not automatically be built. The build must still be passed before it can be merged. Ask one of the team members to make a manual build of this PR. |
|
សូមអរគុណច្រើន! Thank you for your fantastic contribution! I look forward to reviewing this next week with our team and we will provide feedback and comments then. |
mcdurdin
left a comment
There was a problem hiding this comment.
Thank you for this very nice changeset.
I do have a lot of suggestions for the code, so please work through those suggestions -- some of them are applicable throughout, for example avoiding whitespace-only changes.
We have also completed a design review this morning, which is available in a Google Doc: https://docs.google.com/document/d/1ipuuyOGDDC62Wi4mhdkO1SO75vhQCkziybtvb7niAUg/edit?usp=sharing
🩷 Overall we are really happy with how this looks and the improvement to the Keyman user experience. Well done! 🩷
Please feel free to ask questions if anything is unclear!
| implementation 'com.google.android.gms:play-services-maps3d:0.2.0' | ||
| //implementation 'com.google.android.gms:play-services-maps:19.0.0' |
There was a problem hiding this comment.
What is this for? We shouldn't need this in Keyman
| implementation 'com.google.android.gms:play-services-maps3d:0.2.0' | |
| //implementation 'com.google.android.gms:play-services-maps:19.0.0' |
| defaultConfig { | ||
| applicationId "com.tavultesoft.kmapro" | ||
| minSdkVersion 21 | ||
| minSdkVersion 23 |
There was a problem hiding this comment.
We would prefer not to update the minSdkVersion. Is there a reason why it has changed?
| https://support.google.com/googleplay/android-developer/answer/9214102# | ||
| <uses-permission android:name="android.permission.QUERY_ALL_PACKAGES" /> --> | ||
| <application | ||
| android:name=".MyApp" |
There was a problem hiding this comment.
| android:name=".MyApp" |
We're not sure why you've added this name? It's not the name of the app, so if you need it to link to "MyApp.java" we need to rename it to something appropriate.
| <!-- <activity--> | ||
| <!-- android:name=".SplashScreenActivity"--> | ||
| <!-- android:exported="true"--> | ||
| <!-- android:label="@string/app_name"--> | ||
| <!-- android:resizeableActivity="false"--> | ||
| <!-- android:theme="@style/AppTheme_BrandedLunch"--> | ||
| <!-- >--> |
There was a problem hiding this comment.
| <!-- <activity--> | |
| <!-- android:name=".SplashScreenActivity"--> | |
| <!-- android:exported="true"--> | |
| <!-- android:label="@string/app_name"--> | |
| <!-- android:resizeableActivity="false"--> | |
| <!-- android:theme="@style/AppTheme_BrandedLunch"--> | |
| <!-- >--> |
| android:label="@string/app_name" | ||
| android:resizeableActivity="false" | ||
| android:theme="@style/AppTheme.BrandedLaunch"> | ||
| android:theme="@style/Theme.App.Starting"> |
There was a problem hiding this comment.
| android:theme="@style/Theme.App.Starting"> | |
| android:theme="@style/Theme.App.Starting"> |
| keyboardToolbarContainer = findViewById(R.id.keyboardToolbarContainer); | ||
|
|
||
| if (keyboardToolbarToggleButton == null || keyboardToolbarContainer == null) { | ||
| return; |
There was a problem hiding this comment.
Can this ever happen? Do we need to report an error if this is happening?
An error guard like this is nice to have, but it can hide a more serious issue. So we need to decide if this needs to be reported as an error to Keyman team through Sentry.
| keyboardToolbarToggleButton.setOnClickListener(v -> setKeyboardToolbarExpanded(!isKeyboardToolbarExpanded, true)); | ||
|
|
||
| View shareButton = findViewById(R.id.keyboardToolbarShareButton); | ||
| if (shareButton != null) { |
There was a problem hiding this comment.
Do we need to report an error if shareButton == null here (and same for other buttons)?
| private int dpToPx(int dp) { | ||
| float density = getResources().getDisplayMetrics().density; | ||
| return Math.round(dp * density); | ||
| } |
There was a problem hiding this comment.
Is this really needed? Should we be working either exclusively in dp or exclusively in px? I see that it is used only in two places with integer constants in updateKeyboardToolbarPosition:
int keyboardTopOffset = KMManager.getKeyboardHeight(this) + KMManager.getBannerHeight(this) + dpToPx(12);
updateBottomMargin(keyboardToolbarToggleButton, keyboardTopOffset);
updateBottomMargin(keyboardToolbarContainer, dpToPx(8));
| }); | ||
|
|
||
| dialogBuilder.show(); | ||
| // dialogBuilder.show(); no dialog |
There was a problem hiding this comment.
| // dialogBuilder.show(); no dialog |
| setDrawerItemSubtitle(navigationView, R.id.nav_help, | ||
| getString(R.string.drawer_subtitle_about)); |
There was a problem hiding this comment.
The subtitle id does not match the primary title id
|
Note: I committed a couple of minor changes to get the project to build and removed some unnecessary changes from other parts of Keyman. Please pull these changes locally before updating. |
|
Thank you so much for the feedback! I’ve updated the code based on your suggestions and made several improvements. I’ll go through the rest of the comments and continue refining things as much as I can. I’ve also added a Google Doc with a summary of the changes here: https://docs.google.com/document/d/1uJisJ9qhv7sipawU6dWb4HGHbQeoMnKriB50QkU1sxM/edit?usp=sharing . Please let me know if you have any further suggestions! |
Team: @Mengchou97, @seyhachhorn-dev, @phyphy-svg, @kongchanlina, @seanvisal69-ux, and @minanlynna-code.
Test-bot: skip
Fixes: #12718
Updated:
Side panel changes
Dark mode changes
Main page changes
Final looks
Light mode
Dark mode