Missing indicators in pages list #12090
Conversation
|
You can trigger optional UI/connected tests for these changes by visiting CircleCI here. |
|
You can test the changes on this Pull Request by downloading the APK here. |
| } | ||
|
|
||
| private data class ItemUiStateData( | ||
| val labels: List<UiString>, | ||
| @ColorRes val labelsColor: Int?, | ||
| val progressBarUiState: ProgressBarUiState, | ||
| val showOverlay: Boolean, | ||
| val actions: Set<Action> | ||
| val actions: Set<Action>, | ||
| val subtitle: Int? = null |
There was a problem hiding this comment.
thanks, I've fixed that and one other lint error
| @@ -292,14 +292,14 @@ class PageListViewModel @Inject constructor( | |||
| ScheduledPage( | |||
There was a problem hiding this comment.
wdyt about adding named parameters here for the first 3 params? I know it's not necessary but given the subtitle optional param has been placed right below title, it now looks weird to have most of the parameters in named fashion with only the first few with a different notation. Also, comparing these two it looks a lot clearer to have names for everything given it's quite a list of parameters to look into :)
There was a problem hiding this comment.
good idea 👍 , I'll change it in all the places
| @@ -316,14 +316,14 @@ class PageListViewModel @Inject constructor( | |||
| it.remoteId, | |||
There was a problem hiding this comment.
same here if it makes sense to you
| @@ -338,14 +338,14 @@ class PageListViewModel @Inject constructor( | |||
| it.remoteId, | |||
| stringDate | ||
| } else { | ||
| String.format( | ||
| Locale.getDefault(), |
There was a problem hiding this comment.
I've seen we have a very inconsistent use of getLocale() throughout the app.
Sometimes we use Locale.US, sometimes we sue String.format() without indicating the Locale, sometimes Locale.getDefault() as you use here, and sometimes we even have our own LanguageUtils function that does exactly that, called getCurrentDeviceLanguage.
This is just a comment as I was curious to see if there was a particular "way we do things" in the codebase but it seems to be all evenly split. No action need be taken but thought I'd still make the observation here, in case this triggers any thoughts.
There was a problem hiding this comment.
I'm honestly not sure why this method needs a Locale, the documentation says it's for localization. I think we should always use getDefault for user facing texts. We should use Locale.US if we want a consistent result (that's not translated) - this is for example for tracking or for API calls.
I'm also not sure about which way we should call the locale (like the language utils). In this case it was so simple I don't think it needed any utility method.
This PR fixes a missing functionality where there were not indicators for
HomepageandPosts Pageon the pages list. I'm adding these indicators into the field that previously only contained the date the page was created.To test:
To test 2:
PR submission checklist:
RELEASE-NOTES.txtif necessary.