ICT4Dat / ict4dat-news-android

ICT4D.at's App to combine all ICT4D news into one Android application
Apache License 2.0
5 stars 1 forks source link

Refs #127 - Add Toast And Textview #142

Closed rajasone closed 5 years ago

spipau commented 5 years ago

I didn't test your solution, but isn't it way too complicated? I thought this is very easy to solve by just:

  1. In ICT4DNewsFragment add a Toast to

    model.isRefreshing.observe(this, Observer {
            binding.swiperefresh.isRefreshing = it ?: false
            if (it) {
                // Display toast here with "we are connecting you..."
            }
        })

    so it will be displayed every time a new update starts.

  2. Add a TextView behind the Recycler View in the XML Layout and hide it, then do in ICT4DNewsFragment:

model.newsList.observe(this, Observer {
            if (it != null && it.isNotEmpty() && model.searchQuery.isNullOrBlank()) {
                Timber.d("list in fragment: ${it.size}")
                adapter.submitList(it)
                if (model.isRefreshing.value == false) {
                    binding.recyclerview.moveToTop()
                }
            } else {
                // hide here the recycler view and show the text view
            }
        })

Or did I miss something? 😄

rajasone commented 5 years ago

@spipau when we change the device orientation LiveData subscriber get call it's the default behavior of the LiveData, you are absolutely right, I thought why don't we make a good UX experience by showing the toast ONLY once even though user is playing with orientation each second, the above solution is showing the toast only once when user swipe the refresh and our view model is exposing the Mutable live data which is not a good approach so i introduced a bit of the abstraction

spipau commented 5 years ago

@rajasone I understand very much why you did it, but isn't it still an overkill? I would argue that the user won't rotate the phone that often. Is it really necessary to code so much for such a little benefit? Maybe just saving the state in a variable in the ViewModel could solve your issue easier? It would survive the rotation and you don't need any new database requests and so on? Or you work with save instance state in the Fragment, also a valid solution?

rajasone commented 5 years ago

@spipau yes you are absolutely right, let me refactor it to avoid current complex solution and thanks for clearing my confusion :)

spipau commented 5 years ago

I fixed a bug where it always displayed 25 blogs, even if I deactivated some in the Blogs list. I added the German translation and improved some code parts. Great Work @rajasone ! If you agree with the changes, then we can merge this branch 👍