-
Notifications
You must be signed in to change notification settings - Fork 61
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
RUM:4717: Improve CheckableTextViewMapper #2115
RUM:4717: Improve CheckableTextViewMapper #2115
Conversation
229a205
to
2f59c04
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## feature/session-replay/compound-button-mappers #2115 +/- ##
=================================================================================
Coverage ? 69.05%
=================================================================================
Files ? 700
Lines ? 26209
Branches ? 4416
=================================================================================
Hits ? 18098
Misses ? 6899
Partials ? 1212
|
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.
lgtm. Although I don't have a deep knowledge of SR, so a review from someone else who is closer to the Session Replay topic than I am is preferable.
.../com/datadog/android/sessionreplay/internal/recorder/mapper/CheckableCompoundButtonMapper.kt
Outdated
Show resolved
Hide resolved
.../com/datadog/android/sessionreplay/internal/recorder/mapper/CheckableCompoundButtonMapper.kt
Show resolved
Hide resolved
.../com/datadog/android/sessionreplay/internal/recorder/mapper/CheckableCompoundButtonMapper.kt
Show resolved
Hide resolved
...om/datadog/android/sessionreplay/internal/recorder/mapper/BaseCheckableTextViewMapperTest.kt
Outdated
Show resolved
Hide resolved
...om/datadog/android/sessionreplay/internal/recorder/mapper/BaseCheckableTextViewMapperTest.kt
Show resolved
Hide resolved
Agree, @xgouchet @jonathanmos do you want to take a look at this? |
2f59c04
to
e3e842a
Compare
.../com/datadog/android/sessionreplay/internal/recorder/mapper/CheckableCompoundButtonMapper.kt
Outdated
Show resolved
Hide resolved
.../com/datadog/android/sessionreplay/internal/recorder/mapper/CheckableCompoundButtonMapper.kt
Show resolved
Hide resolved
.../com/datadog/android/sessionreplay/internal/recorder/mapper/CheckableCompoundButtonMapper.kt
Show resolved
Hide resolved
8ffda54
to
fa4d075
Compare
fa4d075
to
79dce7f
Compare
8491d83
into
feature/session-replay/compound-button-mappers
What does this PR do?
Summary
This PR improves the session replay
CheckableTextViewMapper
, tries to generate the wireframes which are much closer to the real UI elements.Since
CheckableTextViewMapper
is a base class of the other mappers, following UI components are affected in session replay:Main changes in the PR:
CheckableWireFrameMapper
, mergeresolveCheckedCheckable()
andresolveNotCheckedCheckable()
functions into one functionresolveCheckable
, because we don't generate differentShapeWireframe
for the "checked" or "not checked" states, instead, we retrieve the drawable to generateImageWireframe
.CheckableTextViewMapper
, Inside the implementation ofresolveCheckable
we try to retrieve the drawable of "checked" and "not checked", and then set theColorStateList
of the view on the drawable to have the correct tint.CheckableCompoundButton
, the bounds is adjusted for the drawable rendering,getCheckableDrawable
is implemented to retrieve the drawable for anyCompoundButton
CheckedTextView
getCheckableDrawable
is implemented to retrieve the drawable forCheckedTextView
.ShapeWireframe
is correctly generated, we only need to test thatImageWireFrameHelper.createImageWireframe
is called.Demo
Motivation
Additional Notes
Remain issue
CompoundButtonMapper
for the version under 23Review checklist (to be filled by reviewers)