Stats Widget - add site selector #9929
Conversation
There was a problem hiding this comment.
Thanks for the PR, @planarvoid ! Looks good, I only have a couple of minor comments :)
| alertDialogBuilder.setView(buildView()) | ||
| alertDialogBuilder.setTitle(R.string.stats_widget_select_your_site) | ||
| alertDialogBuilder.setNegativeButton(R.string.cancel) { dialog, _ -> | ||
| dialog?.dismiss() |
There was a problem hiding this comment.
We don't actually have to dismiss dialog manually - negative button should do it by default :)
| @Inject lateinit var viewModelFactory: ViewModelProvider.Factory | ||
| private lateinit var viewModel: ViewsWidgetViewModel | ||
| private fun buildView(): View? { | ||
| val rootView = activity!!.layoutInflater.inflate(R.layout.stats_site_selector, null) |
There was a problem hiding this comment.
I wonder if we should rename the stats_site_selector to something widget related (stats_widget_site_selector?) to avoid confusion with the "main" Stats resources? Same with stats_site_selector_item
There was a problem hiding this comment.
good idea 👍 , I've also renamed the kotlin classes to match the layouts
| ) { | ||
| fun bind(site: SiteUiModel) { | ||
| if (site.iconUrl != null) { | ||
| imageManager.load(itemView.site_icon, ICON, site.iconUrl) |
There was a problem hiding this comment.
I think if we use BLAVATAR instead of ICON the correct placeholder will be set automatically, so we can remove null check and replace this block with something like this:
imageManager.load(itemView.site_icon, BLAVATAR, StringUtils.notNullStr(site.iconUrl))
There was a problem hiding this comment.
I looked into and the BLAVATAR type has R.color.neutral_0 as a placeholder so I think I still have to set it manually to avoid a grey rectangle.
| private val mutableSites = MutableLiveData<List<SiteUiModel>>() | ||
| val sites: LiveData<List<SiteUiModel>> = mutableSites | ||
| private val mutableHideSiteDialog = MutableLiveData<Event<SiteUiModel>>() | ||
| val hideSite: LiveData<Event<SiteUiModel>> = mutableHideSiteDialog |
There was a problem hiding this comment.
Should it be hideSiteDialog instead of hideSite? Also, wanted to ask, why we are storing Event<SiteUiModel> in it instead of Unit with call() method?
There was a problem hiding this comment.
Good idea 👍, I thought Unit defaults to null which wouldn't work
| } | ||
|
|
||
| private fun selectSite(site: SiteUiModel) { | ||
| if (mutableSelectedSite.value != site) { |
There was a problem hiding this comment.
If I'm reading this correctly, this method should not dismiss a dialog when selecting a site that is already selected, yet it does. Wonder if I'm missing something?
There was a problem hiding this comment.
The problem is that the 2 objects are not equal because both have a different anonymous onClick function. I've fixed that but thinking about it now I don't think it really makes sense. We want the user to be able to reselect a site so I'll remove the if
| xmlns:android="http://schemas.android.com/apk/res/android" | ||
| android:layout_width="match_parent" | ||
| android:layout_height="wrap_content" | ||
| android:paddingTop="16dp"> |
There was a problem hiding this comment.
Should we use common values (eg. margin_extra_large) from dimens.xml here and in stats_site_selector_item ?
|
@khaykov thanks a lot for the review! I think I addressed everything so it should be ready for another check. |
This PR adds a site selection dialog to the widget configuration screen. I've created a DialogFragment which communicates directly with the ViewModel and shows a list of sites. When the user selects a site, the dialog is dismissed and the selected site is updated.
The sites show the site icon, title and url.
I've also added a few tests for the ViewModel
To test: