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-5780:Use SurfaceCompoitionGroupMapper to support container components in SR #2182

Merged
merged 2 commits into from
Aug 13, 2024

Conversation

ambushwork
Copy link
Member

What does this PR do?

  • Convert TabRowCompositionGroupMapper to SurfaceCompoitionGroupMapper, because the way it maps the wireframe can be reused in a lot of Jetpack compose container components such as BottomNavigation, TopAppBar, TabRow.

Motivation

RUM-5780

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 requested review from a team as code owners August 12, 2024 12:03
@codecov-commenter
Copy link

codecov-commenter commented Aug 12, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 82.35%. Comparing base (f2ffae5) to head (2a1a50f).

Additional details and impacted files
@@                  Coverage Diff                   @@
##           feature/sr_compose    #2182      +/-   ##
======================================================
+ Coverage               82.30%   82.35%   +0.06%     
======================================================
  Files                     514      514              
  Lines                   18353    18348       -5     
  Branches                 2821     2821              
======================================================
+ Hits                    15104    15110       +6     
+ Misses                   2488     2474      -14     
- Partials                  761      764       +3     
Files Coverage Δ
...compose/internal/mappers/ComposeWireframeMapper.kt 17.28% <100.00%> (+3.18%) ⬆️
.../internal/mappers/SurfaceCompositionGroupMapper.kt 89.47% <100.00%> (ø)

... and 29 files with indirect coverage changes

0xnm
0xnm previously approved these changes Aug 12, 2024
Copy link
Member

@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.

minor doc edit suggestion, otherwise lgtm.

Comment on lines 16 to 21
/**
* This class is responsible to map the Jetpack compose components which have "backgroundColor" & "contentColor"
* in the parameter list, these components are mostly container components such as [BottomNavigation], [TopAppBar]
* and [TabRow], etc.., they usually have a [Surface] implementation under the hood, and can take other composable
* inside.
*/
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
/**
* This class is responsible to map the Jetpack compose components which have "backgroundColor" & "contentColor"
* in the parameter list, these components are mostly container components such as [BottomNavigation], [TopAppBar]
* and [TabRow], etc.., they usually have a [Surface] implementation under the hood, and can take other composable
* inside.
*/
/**
* This class is responsible for the mapping of the Jetpack Compose components which have "backgroundColor" & "contentColor"
* in the parameter list. These components are mostly container components such as [BottomNavigation], [TopAppBar]
* and [TabRow], etc. They usually have a [Surface] implementation under the hood, and can take other composable
* inside.
*/

Copy link
Member Author

Choose a reason for hiding this comment

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

updated

mariusc83
mariusc83 previously approved these changes Aug 13, 2024
@ambushwork ambushwork dismissed stale reviews from mariusc83 and 0xnm via 2a1a50f August 13, 2024 08:06
@ambushwork ambushwork requested review from mariusc83 and 0xnm August 13, 2024 09:23
@ambushwork ambushwork merged commit dcac58d into feature/sr_compose Aug 13, 2024
20 checks passed
@ambushwork ambushwork deleted the yl/fix/compose-button-mapper-crash branch August 13, 2024 09:44
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.

4 participants