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

MOBILE-455: Don't show CTA if custom api keys are used #16

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

alexzfp
Copy link
Collaborator

@alexzfp alexzfp commented Feb 20, 2025

No description provided.

appStorage.save(value, StorageKey.SignupPromptHideTimeMillis)

private suspend fun getCustomApiKeysEnabled() =
appStorage.load(StorageKey.CustomApiKeysEnabled, Boolean::class).get() ?: false
Copy link
Collaborator

Choose a reason for hiding this comment

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

Although it should work, I'd rather consider injecting CustomApiKeysUseCase in this class and checking the state property of it. For the sake of the separation of concerns 🙂 That state is designed to always contain a relevant value.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

np, I just wasn't sure whether there is a convention of injecting classes of same level to each other

Choose a reason for hiding this comment

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

Hi guys attention plzy 5 gmail acomt data sum number apps device haking can posibale recovery strong late pass cl
oud

Copy link
Collaborator Author

@alexzfp alexzfp Feb 24, 2025

Choose a reason for hiding this comment

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

I took an alternative path - instead of injecting one use case with the logic to another usecase with its own logic (which seems to be a bit of code smell for me), I created a unified usecase that joins them. Old usecase was renamed and new combined use case got its old name eliminating the need to change view model(s).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@opof17pro I didn't get you, sorry. Could you elaborate?

Choose a reason for hiding this comment

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

Send me link cloud

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