-
Notifications
You must be signed in to change notification settings - Fork 26
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
fix: blinking keyboard [WPB-4797] #2258
Conversation
Build 907 failed. |
Build 911 succeeded. The build produced the following APK's: |
Codecov Report
@@ Coverage Diff @@
## develop #2258 +/- ##
=============================================
- Coverage 40.51% 40.50% -0.01%
Complexity 1015 1015
=============================================
Files 318 318
Lines 11661 11665 +4
Branches 1550 1551 +1
=============================================
+ Hits 4724 4725 +1
- Misses 6493 6496 +3
Partials 444 444
... and 2 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
APKs built during tests are available here. Scroll down to Artifacts! |
APKs built during tests are available here. Scroll down to Artifacts! |
Build 922 succeeded. The build produced the following APK's: |
APKs built during tests are available here. Scroll down to Artifacts! |
Build 932 succeeded. The build produced the following APK's: |
APKs built during tests are available here. Scroll down to Artifacts! |
Build 955 succeeded. The build produced the following APK's: |
APKs built during tests are available here. Scroll down to Artifacts! |
Build 1011 succeeded. The build produced the following APK's: |
gradle/libs.versions.toml
Outdated
androidx-paging3 = "3.2.1" | ||
androidx-paging3Compose = "3.2.1" |
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.
question: is this necessary for the scope of this PR?
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.
I can create separate PR with this
@@ -73,18 +76,20 @@ fun EnabledMessageComposer( | |||
val density = LocalDensity.current | |||
val navBarHeight = BottomNavigationBarHeight() | |||
val isImeVisible = WindowInsets.isImeVisible | |||
val offsetY = WindowInsets.ime.getBottom(density) | |||
val height = WindowInsets.ime.getBottom(density) |
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.
question: what height is it? I think it was more clear with previous name offsetY
because it's used to move this composable up, but correct me if I'm wrong
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.
You are right, reverted
/** | ||
* Represents an additional option button with controlled interactivity for message composer. | ||
* | ||
* The button's clickability can be controlled via the `isSelected` parameter. This composable also | ||
* internally handles preventing rapid successive clicks using the `enableAgain` variable. This | ||
* mechanism is particularly important to ensure that, during keyboard transitions (expanding or | ||
* collapsing), unintended or unexpected repetitive button clicks do not occur, preventing the | ||
* keyboard from collapsing in an unexpected manner. | ||
* | ||
* @param isSelected Indicates whether the button is selected or not. | ||
* @param onClick The action to be performed when the button is clicked. | ||
* @param modifier The optional [Modifier] to be applied to this composable. | ||
*/ |
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.
suggestion: this comment seems to be out of place because of the const val
in between it and the function that it explains 😅
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.
Moved under this composable 😅
* @param modifier The optional [Modifier] to be applied to this composable. | ||
*/ | ||
|
||
private const val BUTTON_CLICK_DELAY_MILLIS = 500L |
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.
question: does it have to be that big? Half a second is already noticeable and I'm afraid that if we have to add such a long delay then maybe we are doing something wrong
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.
Lowered to 400ms
APKs built during tests are available here. Scroll down to Artifacts! |
Build 1014 succeeded. The build produced the following APK's: |
PR Submission Checklist for internal contributors
The PR Title
SQPIT-764
The PR Description
What's new in this PR?
Issues
On some devices like Xiaomi 10T with android 12. Keyboard is showing for a moment after hiding when is triggered by focus requester or focus manager. I found out that only working solution was to update compose libraries to alpha where readOnly status can force not showing keyboard for BasicTextField
https://developer.android.com/jetpack/androidx/releases/compose-ui#1.6.0-alpha03
Also:
Attachments (Optional)
Attachments like images, videos, etc. (drag and drop in the text box)
PR Post Submission Checklist for internal contributors (Optional)
PR Post Merge Checklist for internal contributors
References
feat(conversation-list): Sort conversations by most emojis in the title #SQPIT-764
.