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

fix #1586: Migrate LinkBankAccountActivity to Compose #1590

Merged

Conversation

devrahul-2508
Copy link
Contributor

@devrahul-2508 devrahul-2508 commented Mar 25, 2024

Issue Fix

Fixes #1586

Screenshots

WhatsApp.Video.2024-03-26.at.9.59.46.PM.mp4

Description

Migrated Link Bank Account Activity and ChooseSimDialog to Compose

  • Apply the AndroidStyle.xml style template to your code in Android Studio.

  • Run the unit tests with ./gradlew check to make sure you didn't break anything

  • If you have multiple commits please combine them into one commit by squashing them.

Copy link
Collaborator

@PratyushSingh07 PratyushSingh07 left a comment

Choose a reason for hiding this comment

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

Also fix your build and format your code using ctrl+shift+L before pushing. Moreover you can remove unused imports using ctrl+shift+O

@devrahul-2508 devrahul-2508 force-pushed the link-bank-account-migration branch from 5f5688a to de9978f Compare March 26, 2024 16:26
Copy link
Collaborator

@PratyushSingh07 PratyushSingh07 left a comment

Choose a reason for hiding this comment

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

Please follow these points as well :

  • Remove extra spaces
  • Use material guidelines
  • resolve conflicts
  • i see changes in xml which shouldn't have happened

color = primaryDarkBlue
)

Spacer(modifier = Modifier.height(15.dp))
Copy link
Collaborator

Choose a reason for hiding this comment

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

change to 16.dp



@HiltViewModel
class LinkBankAccountViewModel @Inject constructor() : ViewModel() {
Copy link
Member

Choose a reason for hiding this comment

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

Inject LocalAssetRepository and fetch banks list from there
image

Copy link
Contributor Author

@devrahul-2508 devrahul-2508 Mar 27, 2024

Choose a reason for hiding this comment

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

@therajanmaurya Here i was getting an error changed the json from {"banks": []} to only []
Screenshot (318)

val bankState = _bankState.asStateFlow()


fun getAllBanks(context: Context) {
Copy link
Member

Choose a reason for hiding this comment

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

fetch flowable banks list from here
image

You can take an example
image

You can have a query field

    private val _bankSearchquery = MutableStateFlow<String>>("")
    var bankSearchquery = _bankSearchquery.asStateFlow()
    
fun updateBankSearchquery(query: String) {
_bankSearchquery.update { query }
} 

 val banks: StateFlow<List<Bank>> = combine(
        localAssetRepository.getBanks(),
        bankSearchquery,
        ::Pair
    )
        .map { 
            val countries = it.first
            signupData = signupData.copy(
                countryId = countries.find { it.name == signupData.countryName }?.id ?: ""
            )
           val allBanks =  it.first.map { Bank (it, R.drawable.ic_bank, 1)}.filter { it.name.contains(it.second) }
            
             popularBanks = // add isPopular field in Bank set them above true if below bank name matches so you can filter with single source and show on ui
             
            LoadedBankState(
              allBanks = allBanks, 
       
           )
        }
        .stateIn(
            scope = viewModelScope,
            started = SharingStarted.WhileSubscribed(5_000),
            initialValue = emptyList(),
        )

Copy link
Member

Choose a reason for hiding this comment

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

using this implementation you don't need other fields as user type and you update query, it will automatically update the bank list.

Copy link
Contributor Author

@devrahul-2508 devrahul-2508 Mar 27, 2024

Choose a reason for hiding this comment

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

@therajanmaurya here on init i am getting the banks like this. The popular banks is a separate thing in ui and some of the popular banks is not present in json. the popular banks have their own drawable as u can see
Screenshot (323)

After that as per your search logic i am setting the filtered banks and the popular banks
Screenshot (321)

Please let me know if any changes required.

@devrahul-2508 devrahul-2508 force-pushed the link-bank-account-migration branch 5 times, most recently from 3a52a60 to db3808b Compare March 27, 2024 19:00
@devrahul-2508 devrahul-2508 force-pushed the link-bank-account-migration branch from bd5fc00 to 839bc9c Compare April 8, 2024 17:37
@devrahul-2508 devrahul-2508 force-pushed the link-bank-account-migration branch from 839bc9c to ea6eb48 Compare April 8, 2024 18:33
@therajanmaurya therajanmaurya merged commit 71a9502 into openMF:dev Apr 13, 2024
2 of 4 checks passed
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.

Migrate LinkBankAccountActivity to Compose
3 participants