Repository navigation
This commit will add a install keyboards button to the tutorial screen - #762
henrikth93 wants to merge 4 commits into
Conversation
andrewtavis
added
the
no-changelog
|
Thanks for the PR, @henrikth93! I don't think we need a changelog entry here, so skipping it :) |
|
Added |
Updated changelog
andrewtavis
removed
the
no-changelog
| - An in-app tutorial is provided to detail functionalities of the application ([#602](https://github.com/scribe-org/Scribe-Android/issues/602), [#615](https://github.com/scribe-org/Scribe-Android/issues/615), [#616](https://github.com/scribe-org/Scribe-Android/issues/616)). | ||
| - The user is able to easily rate the application, with installer-aware routing to the appropriate store ([#165](https://github.com/scribe-org/Scribe-Android/issues/165), [#640](https://github.com/scribe-org/Scribe-Android/issues/640)). | ||
|
|
||
| - If the user does not have any keyboards installed, the tutorial screen will not show tutorial content, but will instead give the user access to an install keyboard-button. |
There was a problem hiding this comment.
note: Can we add a space after this entry and the issue number as is done in other changelog entries, @henrikth93?
| val secondaryTextColor = MaterialTheme.colorScheme.onSurface.copy(alpha = 0.6f) | ||
| val dividerColor = MaterialTheme.colorScheme.outlineVariant | ||
| val headerColor = MaterialTheme.colorScheme.onBackground | ||
| val isKeyboardInstalled by viewModel.isKeyboardInstalled.collectAsState() |
There was a problem hiding this comment.
i guess would be better to have isKeyboardInstalled.collectAsStateWithLifecycle() which is the modern recommended Android standard? @henrikth93 @andrewtavis
There was a problem hiding this comment.
additionally we need to add a lifecycle observer here similar to the implementation in SettingsScreen.kt to refresh the status, as Android doesn't broadcast keyboard changes by default
There was a problem hiding this comment.
i guess would be better to have isKeyboardInstalled.collectAsStateWithLifecycle() which is the modern recommended Android standard? @henrikth93 @andrewtavis
i noticed rest of codebase uses collectAsState only , so i guess this isn't mandatory or we could have a separate issue updating everything to collectAsStateWithLifecycle
There was a problem hiding this comment.
I'd do a separate issue to update the whole codebase, @ashb155 :) Do you want to make that?
There was a problem hiding this comment.
Agreed with @andrewtavis. Since the entire app currently uses collectAsState(), keeping it consistent here is fine. We can migrate everything to collectAsStateWithLifecycle() in a separate codebase-wide issue.
There was a problem hiding this comment.
Would one of you want to make the issue?
|
Note that the change in #755 is likely quite similar to what we need here :) |
This commit will add lifecycle to the tutorialhomescreen
prince-0408
left a comment
prince-0408
left a comment
There was a problem hiding this comment.
Thanks for working on this @henrikth93! The lifecycle refresh and keyboard installation check look solid. 👍
I've left a couple of minor inline suggestions to align with the download data UI and issue #752.
| horizontalAlignment = Alignment.CenterHorizontally, | ||
| ) { | ||
| Text( | ||
| text = stringResource(R.string.i18n_app_download_menu_ui_no_keyboards_installed), |
There was a problem hiding this comment.
To match the download data UI (DataDownloadScreen.kt) and issue #752, could we wrap this message inside a Card (or Surface) with rounded corners? That way it looks consistent with the rest of the tutorial cards on this screen.
| ) | ||
| } | ||
| // Install keyboards | ||
| InstallKeyboardButton( |
There was a problem hiding this comment.
Could we add Spacer(modifier = Modifier.weight(1f)) before InstallKeyboardButton? This will keep the primary action button pinned to the bottom of the screen, matching the layout when keyboards are installed.
| - Vibrate on keypress and key click functionalities are included ([#405](https://github.com/scribe-org/Scribe-Android/issues/405), [#406](https://github.com/scribe-org/Scribe-Android/issues/406)). | ||
| - An in-app tutorial is provided to detail functionalities of the application ([#602](https://github.com/scribe-org/Scribe-Android/issues/602), [#615](https://github.com/scribe-org/Scribe-Android/issues/615), [#616](https://github.com/scribe-org/Scribe-Android/issues/616)). | ||
| - The user is able to easily rate the application, with installer-aware routing to the appropriate store ([#165](https://github.com/scribe-org/Scribe-Android/issues/165), [#640](https://github.com/scribe-org/Scribe-Android/issues/640)). | ||
| - If the user does not have any keyboards installed, the tutorial screen will not show tutorial content, but will instead give the user access to an install keyboard-button. |
There was a problem hiding this comment.
As @andrewtavis mentioned earlier, we can revert this changelog addition since this change doesn't require a changelog entry.
andrewtavis
added
the
no-changelog
|
Thanks for the further review, @prince-0408! |
… spacer before the installkeyboardbutton This commit will wrap the download text inside a card, and also add a spacer before the installkeyboardbutton. Also, it will remove the added line in the changelog.
Contributor checklist
./gradlew lintKotlin detekt testcommand as directed in the testing section of the contributing guideDescription
This commit will add an install keyboard button to the tutorial screen which is shown if the user has no installed keyboards.
Related issue