Skip to content
This repository has been archived by the owner on Jun 20, 2023. It is now read-only.

Add Local Incidence Card to Statistics (EXPOSUREAPP-7446) #3596

Merged
merged 35 commits into from
Jul 13, 2021

Conversation

kolyaopahle
Copy link
Contributor

Description

This PR Adds the local incidence card for the new local statistics feature, note that there is no logic in place to actually display the card at the moment this will be done in a follow up PR

@kolyaopahle kolyaopahle added ui Issue related to UI aspects maintainers Tag pull requests created by maintainers labels Jul 1, 2021
@kolyaopahle kolyaopahle added this to the 2.6.0 milestone Jul 1, 2021
@kolyaopahle kolyaopahle requested review from a team July 1, 2021 12:25
NataliaLemmerth
NataliaLemmerth previously approved these changes Jul 1, 2021
Copy link
Contributor

@NataliaLemmerth NataliaLemmerth left a comment

Choose a reason for hiding this comment

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

UA approved

@jurajkusnier jurajkusnier self-assigned this Jul 1, 2021
@d4rken d4rken self-assigned this Jul 2, 2021
@kolyaopahle kolyaopahle marked this pull request as draft July 5, 2021 08:21
harambasicluka and others added 3 commits July 7, 2021 13:42
… cards,

district names are resolved in the parser,
also added fetching of local statistics based on active districts
@kolyaopahle kolyaopahle marked this pull request as ready for review July 11, 2021 15:36
Copy link
Contributor

@jurajkusnier jurajkusnier left a comment

Choose a reason for hiding this comment

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

I got crash after removing Local Incidence Card

    java.lang.IllegalStateException: Two different ViewHolders have the same stable ID. Stable IDs in your adapter MUST BE unique and SHOULD NOT change.
    ViewHolder 1:LocalIncidenceCard{312f8db position=2 id=8, oldPos=-1, pLpos:-1 not recyclable(1)} 
     View Holder 2:LocalIncidenceCard{ddbd4cd position=1 id=8, oldPos=-1, pLpos:-1} androidx.recyclerview.widget.RecyclerView{afd4cff VFED..... ......ID 0,269-1080,1241 #7f0a06d7 app:id/statistics_recyclerview}, adapter:de.rki.coronawarnapp.statistics.ui.homecards.StatisticsCardAdapter@9e5d0c4, layout:de.rki.coronawarnapp.statistics.ui.homecards.StatisticsLayoutManager@80dcbad, context:de.rki.coronawarnapp.ui.main.MainActivity@a39276d

@jurajkusnier
Copy link
Contributor

jurajkusnier commented Jul 12, 2021

Doesn't meet the Acceptance Criteria:

  • The user can add up to 5 additional tiles for local incidences. As long as less than 5 tiles have been added, the tile shows a button to add a new local incidence

The code is already there but is not used. Something like this should work:

// HomeFragmentViewModel.kt, line 203 
data = statsData.copy(
   items = mutableListOf(AddStatsItem(isEnabled = statsData.items.filterIsInstance<LocalIncidenceStats>().size < 5))
                                .plus(statsData.items)
                        ),

@jurajkusnier
Copy link
Contributor

Doesn't meet the Acceptance Criteria:

  • if a county was selected, a new tile for the selected county is added left to the latest added local incidence tile. The first local incidence tile is added left to the national incidence tile

@kolyaopahle
Copy link
Contributor Author

I got crash after removing Local Incidence Card

    java.lang.IllegalStateException: Two different ViewHolders have the same stable ID. Stable IDs in your adapter MUST BE unique and SHOULD NOT change.
    ViewHolder 1:LocalIncidenceCard{312f8db position=2 id=8, oldPos=-1, pLpos:-1 not recyclable(1)} 
     View Holder 2:LocalIncidenceCard{ddbd4cd position=1 id=8, oldPos=-1, pLpos:-1} androidx.recyclerview.widget.RecyclerView{afd4cff VFED..... ......ID 0,269-1080,1241 #7f0a06d7 app:id/statistics_recyclerview}, adapter:de.rki.coronawarnapp.statistics.ui.homecards.StatisticsCardAdapter@9e5d0c4, layout:de.rki.coronawarnapp.statistics.ui.homecards.StatisticsLayoutManager@80dcbad, context:de.rki.coronawarnapp.ui.main.MainActivity@a39276d

weird, this has not happened for me during testing, will investigate

Copy link
Member

@d4rken d4rken left a comment

Choose a reason for hiding this comment

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

Looking better every day 😁.
Found a few inconsistencies we need to check out.

@harambasicluka harambasicluka added the prio PRs to review first. label Jul 13, 2021
@jurajkusnier
Copy link
Contributor

one minor thing: AddCard should be probably completely disabled when there is more than 5 LocalIncidenceCard. So also the ripple effect shouldn't be there.

Disabled the AddCard completely (no ripple)
Copy link
Contributor

@jurajkusnier jurajkusnier left a comment

Choose a reason for hiding this comment

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

The positive trend icon should be green and the negative trend icon should be red, like the rest of the cards. But I see the trend icon only in grey color.

@sonarcloud
Copy link

sonarcloud bot commented Jul 13, 2021

Kudos, SonarCloud Quality Gate passed!

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

9.7% 9.7% Coverage
0.0% 0.0% Duplication

@harambasicluka harambasicluka merged commit aff93e4 into release/2.6.x Jul 13, 2021
@harambasicluka harambasicluka deleted the feature/7446-local-incidence-card branch July 13, 2021 14:20
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
maintainers Tag pull requests created by maintainers prio PRs to review first. ui Issue related to UI aspects
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants