Skip to content

Feature/bma/fix typing #6072

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

Merged
merged 4 commits into from
May 17, 2022
Merged

Feature/bma/fix typing #6072

merged 4 commits into from
May 17, 2022

Conversation

bmarty
Copy link
Member

@bmarty bmarty commented May 17, 2022

Type of change

  • Feature
  • Bugfix
  • Technical
  • Other :

Content

Motivation and context

Fixes #6063
Also fixes #5632

Screenshots / GIFs

Before After
Screenshot 2022-05-17 at 11 13 53Screenshot 2022-05-17 at 11 14 28 Screenshot 2022-05-17 at 11 55 40Screenshot 2022-05-17 at 11 53 25

Tests

  • Smoke test on typing, and scrolling the timeline

Tested devices

  • Physical
  • Emulator
  • OS version(s):

Checklist

android:layout_width="wrap_content"
android:layout_height="wrap_content"
app:barrierDirection="top"
app:constraint_referenced_ids="composerLayout,notificationAreaView,failedMessagesWarningStub" />
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this was duplicated code with badgeBarrier which has been renamed to bottomBarrier

@bmarty bmarty added the Z-NextRelease For issues and PRs which should be included in the NextRelease. label May 17, 2022
@bmarty bmarty requested review from a team, ouchadam and fedrunov and removed request for a team May 17, 2022 10:01
android:paddingStart="20dp"
android:paddingEnd="20dp"
app:layout_constraintBottom_toTopOf="@id/composerLayout"
app:layout_constraintBottom_toTopOf="@id/bottomBarrier"
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This view is now on top of the barrier, and the timeline is now on top of this view.

const val OVERLAP_FACT0R = -3 // =~ 30% to left
}

private val typingAvatarSize by lazy {
Copy link
Contributor

@ouchadam ouchadam May 17, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if render is always called from the main thread we could use the unsafe lazy initializer to avoid a synchronised call

@DoM1niC DoM1niC mentioned this pull request May 17, 2022

<!-- This is a LinearLayout which will contain avatars of the typing users -->
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this comment feels like it could be replaced by renaming the id to typingUsersAvatars

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point, done

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(I keep the comment since I added it for clarity when looking at the layout. Since there is no avatar preview)

@ouchadam
Copy link
Contributor

is it intentional for the typing indicator to no longer overlap the content but instead provide its own background? (to double check if this was missing from the original implementation or intended from a design perspective)

@github-actions
Copy link

github-actions bot commented May 17, 2022

Unit Test Results

122 files  ±0  122 suites  ±0   1m 59s ⏱️ -21s
205 tests ±0  205 ✔️ ±0  0 💤 ±0  0 ±0 
690 runs  ±0  690 ✔️ ±0  0 💤 ±0  0 ±0 

Results for commit 5df0488. ± Comparison against base commit 3674ae7.

♻️ This comment has been updated with latest results.

@bmarty
Copy link
Member Author

bmarty commented May 17, 2022

is it intentional for the typing indicator to no longer overlap the content but instead provide its own background? (to double check if this was missing from the original implementation or intended from a design perspective)

For the original design, there is a Figma link in #3296.

I think the design wanted the typing notification not to be visible when the timeline is scrolled. But implementing that is out of scope of this PR.

@ouchadam
Copy link
Contributor

I think the design wanted the typing notification not to be visible when the timeline is scrolled. But implementing that is out of scope of this PR.

probably worth a ticket so we don't forget to revisit 🤞

@bmarty bmarty merged commit 57ae714 into develop May 17, 2022
@bmarty bmarty deleted the feature/bma/fix_typing branch May 17, 2022 13:34
@github-actions
Copy link

Matrix SDK

Integration Tests Results:

  • [org.matrix.android.sdk.session]
    = passed=7 failures=13 errors=0 skipped=3
  • [org.matrix.android.sdk.account]
    = passed=1 failures=2 errors=0 skipped=2
  • [org.matrix.android.sdk.internal]
    = passed=88 failures=59 errors=0 skipped=13
  • [org.matrix.android.sdk.ordering]
    = passed=16 failures=0 errors=0 skipped=0
  • [org.matrix.android.sdk.PermalinkParserTest]
    = passed=2 failures=0 errors=0 skipped=0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Z-NextRelease For issues and PRs which should be included in the NextRelease.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Is typing UI bricked Typing indicator appears cut off in bigger fonts
2 participants