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

Instrument metrics for iOS for the IA project #5401 #5836

Merged
merged 2 commits into from
Mar 17, 2022

Conversation

gileluard
Copy link
Contributor

fix #5401

@gileluard gileluard requested review from a team and pixlwave and removed request for a team March 16, 2022 16:00
@github-actions
Copy link

github-actions bot commented Mar 16, 2022

📱 Scan the QR code below to install the build for this PR.
🔒 This build is for internal testing purpose. Only devices listed in the ad-hoc provisioning profile can install Element Alpha.

QR code

If you can't scan the QR code you can install the build via this link: https://i.diawi.com/Ae9xZt

Copy link
Member

@pixlwave pixlwave left a comment

Choose a reason for hiding this comment

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

Looks good to me. One small comment and a potential (larger) suggestion inline.

// MARK: - Constants

private enum Constants {
static let lastNumberOfSpaces: String = "AnalyticsSpaceTracker.lastNumberOfSpaces"
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
static let lastNumberOfSpaces: String = "AnalyticsSpaceTracker.lastNumberOfSpaces"
static let lastNumberOfSpacesKey: String = "AnalyticsSpaceTracker.lastNumberOfSpaces"

// Last number of spaces tracked
private var lastNumberOfSpaces: Int? {
get {
guard let value = UserDefaults.standard.value(forKey: Constants.lastNumberOfSpaces) as? NSNumber else {
Copy link
Member

Choose a reason for hiding this comment

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

Probably not important for now as I think the space service only runs in the main target, but it might potentially be useful to store this in RiotSettings or RiotSettings.defaults so that it is available in another target if ever needed.

For the FTUEUseCaseSelection I added a UserSessionProperties class that we can add user properties to on a per user basis (pre-planning for multi-account at some point) and it will store them together. Might be possible for you to use that if you wanted.

Copy link
Contributor Author

@gileluard gileluard Mar 17, 2022

Choose a reason for hiding this comment

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

Good point. My first thought was to not make growing RiotSettings too much but I forgot about targets. I'll make the update.

@gileluard gileluard requested a review from pixlwave March 17, 2022 09:49
Copy link
Member

@pixlwave pixlwave left a comment

Choose a reason for hiding this comment

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

Looks good to me 👍

@gileluard gileluard merged commit 164be25 into develop Mar 17, 2022
@gileluard gileluard deleted the gil/5401_Instrument_metrics_for_the_IA_project branch March 17, 2022 10: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.

Instrument metrics for iOS for the IA project
2 participants