Skip to content

Conversation

@agustinkoll-rootstrap
Copy link
Contributor

@agustinkoll-rootstrap agustinkoll-rootstrap commented Aug 15, 2023

Issue:

#19

closes #19

Description

Added base view model in the app and in the example app modules.
Added interface for uiState

Copy link
Collaborator

@amaury901130 amaury901130 left a comment

Choose a reason for hiding this comment

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

💪

Copy link
Contributor

@hrodrick hrodrick left a comment

Choose a reason for hiding this comment

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

Nice work! Just left a comment

.matches() || password.isEmpty()
updateUiState { it.copy(password = password, showPasswordError = !isPasswordValid) }
}

Copy link
Collaborator

@amaury901130 amaury901130 Aug 16, 2023

Choose a reason for hiding this comment

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

probably you can add a function here:

private fun validateInput(input: String, regex: Regex): Boolean {
  return regex.matches(input) || input.isEmpty() 
}

to avoid duplicating the validation checks, wdyt?

Copy link
Collaborator

@amaury901130 amaury901130 Aug 16, 2023

Choose a reason for hiding this comment

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

The validation logic runs every time the user types, this could be inefficient if validating too frequently
You could debounce the validation to only run after the user stops typing or only validate when the onLogingButtonClicked is called.
wdyt?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll leave a todo

import kotlinx.coroutines.flow.update
import kotlinx.coroutines.launch

open class BaseViewModel<UiStateType : UiState, NavigationEventT : Any>(
Copy link
Collaborator

@amaury901130 amaury901130 Aug 16, 2023

Choose a reason for hiding this comment

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

I think this class could be abstract

@agustinkoll-rootstrap agustinkoll-rootstrap merged commit 131e2a7 into main Aug 24, 2023
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.

State Management

6 participants