Skip to content

RUM-6871: Add Material Chip mapper and improve CompoundButton telemetry#2364

Merged
ambushwork merged 1 commit into
developfrom
yl/sr-mnt/add-chip-support
Oct 31, 2024
Merged

RUM-6871: Add Material Chip mapper and improve CompoundButton telemetry#2364
ambushwork merged 1 commit into
developfrom
yl/sr-mnt/add-chip-support

Conversation

@ambushwork

@ambushwork ambushwork commented Oct 31, 2024

Copy link
Copy Markdown
Member

What does this PR do?

The telemetry in CompoundButton was not clear enough to understand why we can't resolve buttonDrawable of the view prior to this PR, so one of the task is to append the view information in the telemetry so that we can know the view type.

ChipWireframeMapper is added to support Material Chip component since it's a widely used one on customer side.

Motivation

RUM-6871

Demo

Sample App Session Replay Allow Session Replay Mask
image image image

Review checklist (to be filled by reviewers)

  • Feature or bugfix MUST have appropriate tests (unit, integration, e2e)
  • Make sure you discussed the feature or bugfix with the maintaining team in an Issue
  • Make sure each commit and the PR mention the Issue number (cf the CONTRIBUTING doc)

@ambushwork
ambushwork force-pushed the yl/sr-mnt/add-chip-support branch from f83c4c1 to 356c9ea Compare October 31, 2024 11:18
@codecov-commenter

codecov-commenter commented Oct 31, 2024 •
edited
Loading

Copy link
Copy Markdown

Codecov Report

Attention: Patch coverage is 70.21277% with 14 lines in your changes missing coverage. Please review.

Project coverage is 70.35%. Comparing base (a1b97be) to head (92876f8).
Report is 4 commits behind head on develop.

Files with missing lines Patch % Lines
...l/recorder/mapper/CheckableCompoundButtonMapper.kt 14.29% 10 Missing and 2 partials ⚠️
...ionreplay/material/internal/ChipWireframeMapper.kt 92.31% 1 Missing and 1 partial ⚠️
Additional details and impacted files 
@@             Coverage Diff             @@
##           develop    #2364      +/-   ##
===========================================
+ Coverage    70.26%   70.35%   +0.09%     
===========================================
  Files          743      744       +1     
  Lines        27681    27719      +38     
  Branches      4629     4631       +2     
===========================================
+ Hits         19449    19501      +52     
+ Misses        6937     6932       -5     
+ Partials      1295     1286       -9
Files with missing lines Coverage Δ
...sessionreplay/material/MaterialExtensionSupport.kt 97.14% <100.00%> (+0.59%) ⬆️
...id/sessionreplay/recorder/mapper/TextViewMapper.kt 90.00% <ø> (ø)
...ionreplay/material/internal/ChipWireframeMapper.kt 92.31% <92.31%> (ø)
...l/recorder/mapper/CheckableCompoundButtonMapper.kt 39.06% <14.29%> (+1.13%) ⬆️

... and 26 files with indirect coverage changes

@ambushwork
ambushwork marked this pull request as ready for review October 31, 2024 12:41
@ambushwork
ambushwork requested review from a team as code owners October 31, 2024 12:41
0xnm
0xnm approved these changes Oct 31, 2024
View reviewed changes

@0xnm 0xnm left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

lgtm, but text on the chip is centered on the application screenshot, but it is not centered on the SR screenshot. Is it something to fix?

Comment thread ...terial/src/test/kotlin/com/datadog/android/sessionreplay/material/ChipWireframeMapperTest.kt Outdated
@ambushwork
ambushwork force-pushed the yl/sr-mnt/add-chip-support branch from 356c9ea to 92876f8 Compare October 31, 2024 13:01
@ambushwork

ambushwork commented Oct 31, 2024

Copy link
Copy Markdown
Member Author

@0xnm Actually the text position of the chip in sample app is LEFT not CENTER, it's just the chip is wrapping the content makes it looks like it is in center.

And reason why in Session Replay it looks different, it's because that we can not correctly parse the font of the text which makes it look smaller than the real one. That's is a legacy issue we haven't been able to fix.

@ambushwork
ambushwork merged commit b83e7b6 into develop Oct 31, 2024
@ambushwork
ambushwork deleted the yl/sr-mnt/add-chip-support branch October 31, 2024 14:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@0xnm 0xnm 0xnm approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@ambushwork @codecov-commenter @0xnm