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-4151 [SR] Support background color for Tab Bars #1890

Merged
merged 6 commits into from
Jun 11, 2024

Conversation

mariedm
Copy link
Member

@mariedm mariedm commented Jun 10, 2024

What and why?

Session Replay currently does not support custom background colors for tab bars, defaulting to white or black based on the user's trait collection.

How?

This PR:

  • Modifies the SR Tab Bar Recorder to support custom background colors.
  • Adds new snapshot tests for both static and embedded tab bars.

This work is similar to a previous PR that added support for custom background colors on navigation bars. For more context, see PR #1864.

Notes on deselected items’ Tint Color

TLDR: if no unselectedTintColor is set, icons will be rendered in white.

  • Both selected and unselected tint colors for tab bar items can be set using tintColor and unselectedItemTintColor.
  • If unselectedItemTintColor is not set, the default is gray, though it appears white in the view debugger and is not accessible via code.
  • An item's icon is recorded using the UIImageViewRecorder. The correct tint color for a selected item is obtained through the tintColorProvider.
  • It is not possible to detect selection status at the image or imageView level within the UIImageViewRecorder.
    • image.isTinted always returns true if a tintColor is set, regardless of selection status.
    • Without access to the unselected tint color, we cannot determine the rendered color for unselected items.
    • Comparing the recorded image with the withTintColor(_:) effect applied to it does not work.
    • Accessing the Tab Bar and Tab Bar button from the image's superviews is possible, but the button is of type UITabBarButton, a private API, preventing state checks to determine selection status and appropriate tint color.

Snapshot tests screenshots

Static Tab Bar

testTabBars()-allow-privacy

Embedded Tab Bar

testTabBars()-embeddedtabbar-allow-privacy

Review checklist

  • Feature or bugfix MUST have appropriate tests (unit, integration)
  • Make sure each commit and the PR mention the Issue number or JIRA reference
  • Add CHANGELOG entry for user facing changes

Custom CI job configuration (optional)

  • Run unit tests for Core, RUM, Trace, Logs, CR and WVT
  • Run unit tests for Session Replay
  • Run integration tests
  • Run smoke tests
  • Run tests for tools/

@mariedm mariedm marked this pull request as ready for review June 10, 2024 10:58
@mariedm mariedm requested review from a team as code owners June 10, 2024 10:58
ncreated
ncreated previously approved these changes Jun 11, 2024
Copy link
Member

@ncreated ncreated left a comment

Choose a reason for hiding this comment

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

Looks great 🎯. I appreciate the level of details in PR description and snapshot tests - all clear 🚀!

Minor comment on conventions: we usually mention the JIRA ref (RUM-4151 in this case) as part of every commit (prefix) and in the PR title. This way we establish automatic link between GH and JIRA, which helps tracking decisions later. For this PR, let's mention it in the title at least 🙂.

@@ -25,7 +25,6 @@ internal struct UITabBarRecorder: NodeRecorder {
}

private func inferOccupiedFrame(of tabBar: UITabBar, in context: ViewTreeRecordingContext) -> CGRect {
// TODO: RUMM-2791 Enhance appearance of `UITabBar` and `UINavigationBar` in SR
Copy link
Member

Choose a reason for hiding this comment

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

🎯👌

Copy link
Member

@maciejburda maciejburda left a comment

Choose a reason for hiding this comment

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

Nice!

I would only suggest defaulting the icon image tint to gray whenever we deal with unselectedItemTintColor == nil.

Also - we probably can drop the bottom border. Seems like native component has only a gray line on top.

maxep
maxep previously approved these changes Jun 11, 2024
Copy link
Member

@maxep maxep left a comment

Choose a reason for hiding this comment

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

Looks great! nice work on the snapshot tests 👏

@mariedm mariedm changed the title [SR] Support background color for Tab Bars RUM-4151 [SR] Support background color for Tab Bars Jun 11, 2024
@mariedm
Copy link
Member Author

mariedm commented Jun 11, 2024

Minor comment on conventions: we usually mention the JIRA ref (RUM-4151 in this case) as part of every commit (prefix) and in the PR title. This way we establish automatic link between GH and JIRA, which helps tracking decisions later. For this PR, let's mention it in the title at least 🙂.

Got it! I will follow this pattern next time. In the meantime, I added it to the PR title.

@mariedm
Copy link
Member Author

mariedm commented Jun 11, 2024

I would only suggest defaulting the icon image tint to gray whenever we deal with unselectedItemTintColor == nil.

Also - we probably can drop the bottom border. Seems like native component has only a gray line on top.

@maciejburda I can remove the bottom border color. 👍 However, how do you suggest we default the icon to gray when unselectedItemTintColor == nil? We don't have access to this property within the UIImageViewRecorder, and we can't determine if the tab bar item it belongs to is selected or not. Therefore, we don't know whether to apply the tintColor or the unselectedItemTintColor.

@mariedm
Copy link
Member Author

mariedm commented Jun 11, 2024

@maciejburda Actually, the current implementation in UITabBarRecorder will also remove the top border, which might not be what we want?

testTabBars()-mask-privacy

testTabBars()-embeddedtabbar-allow-privacy

@mariedm mariedm dismissed stale reviews from maxep and ncreated via 5778c80 June 11, 2024 09:56
@datadog-datadog-prod-us1
Copy link

Datadog Report

Branch report: mariedm/feat/RUM-4151
Commit report: 9a9d00a
Test service: dd-sdk-ios

✅ 0 Failed, 253 Passed, 0 Skipped, 14.05s Total Time

@mariedm
Copy link
Member Author

mariedm commented Jun 11, 2024

@maciejburda as discussed in the Daily today, I can merge this PR once I have the required approvals to unblock @ncreated's work. I created a follow-up ticket from your comment.

@mariedm mariedm requested review from maciejburda and maxep June 11, 2024 14:20
Copy link
Member

@maciejburda maciejburda left a comment

Choose a reason for hiding this comment

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

LGTM!

As discussed - we can try to tackle white tint images in a separate PR

@mariedm mariedm merged commit f33ccb7 into develop Jun 11, 2024
4 checks passed
@mariedm mariedm deleted the mariedm/feat/RUM-4151 branch June 11, 2024 14:42
This was referenced Jun 12, 2024
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