Skip to content

[Customer Center] UI polish for empty content#4147

Merged
codykerns merged 5 commits into
integration/customer_support_workflowfrom
cody/customer_center_polish
Aug 8, 2024
Merged

[Customer Center] UI polish for empty content#4147
codykerns merged 5 commits into
integration/customer_support_workflowfrom
cody/customer_center_polish

Conversation

@codykerns

@codykerns codykerns commented Aug 6, 2024

Copy link
Copy Markdown
Member

Attempting some UI polish

  • Implements ContentUnavailableView for the views that have 'empty' content (with a visually similar backwards compatible version)
  • Improves instructions for each platform for wrong-platform subscription management
New Old
New Version Old Version

@codykerns codykerns marked this pull request as ready for review August 6, 2024 20:26
@codykerns codykerns requested review from aboedo and vegaro August 6, 2024 20:27
@aboedo

aboedo commented Aug 6, 2024

Copy link
Copy Markdown
Member

The code is :chefskiss: and I love how the view looks! Thanks for doing this Cody!

We'll have to update snapshots for this (@RevenueCat/coresdk could you remind me how that's done?).

I think the only thing that we'll have to figure out is how to make the strings used here localizable, do we already have any of those coming from the backend that we can reuse? (Also question for CoreSDK)

@aboedo aboedo requested a review from a team August 6, 2024 20:30

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

Love it! 🤩

Regarding localized strings, I believe the idea is to place them in CustomerCenterConfigData.Localization.
Not sure how to update the snapshots though.
(cc @aboedo)

@tonidero

tonidero commented Aug 7, 2024

Copy link
Copy Markdown
Contributor

This looks nice!

We'll have to update snapshots for this

It's been a while but I believe you should be able to trigger it through either fastlane (bundle exec fastlane generate_snapshots_RCUI) or CircleCI (selecting this branch in CircleCI, then triggering a new job with a booleean generate_revenuecatui_snapshots parameter set to true). Lmk if I can help with this!

As for localization, what @JayShortway mentioned is correct! The idea is to have them as common strings which can be overriden by the backend response, but still have some default in the SDK.

@tonidero

tonidero commented Aug 7, 2024

Copy link
Copy Markdown
Contributor

Actually, I believe we don't have snapshot tests for CustomerCenter yet right @aboedo? I believe we are planning to add them but didn't yet

@codykerns

Copy link
Copy Markdown
Member Author

Thanks all!

@aboedo since this PR just refactors the already hard-coded strings, should the localization changes be made in a separate PR?

@aboedo

aboedo commented Aug 8, 2024

Copy link
Copy Markdown
Member

My bad, I'd missed that the strings already existed and that we don't even have snapshots for customer center.
This is why we shouldn't let me review things, feel free to merge once tests pass! Looks like CI doesn't like something in the availability checks mentioning visionOS?

@codykerns

Copy link
Copy Markdown
Member Author

Since the failing test was related to snapshots (that I ran manually), and affects all of customer center currently and not just this PR, going to merge this. Standard tests are succeeding 👍

@codykerns codykerns merged commit c52f3cf into integration/customer_support_workflow Aug 8, 2024
@codykerns codykerns deleted the cody/customer_center_polish branch August 8, 2024 16:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants