Skip to content
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-6871: Add Material Chip mapper and improve CompoundButton telemetry #2364

Merged
merged 1 commit into from
Oct 31, 2024

Conversation

ambushwork
Copy link
Contributor

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)

@codecov-commenter
Copy link

codecov-commenter commented Oct 31, 2024

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
Copy link
Contributor

@0xnm 0xnm left a comment

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?

@ambushwork
Copy link
Contributor 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
24 checks passed
@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
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants