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: [ANDROAPP-5663] Tei dashboard event list scrolling getting clipped #3433

Conversation

msasikanth
Copy link
Contributor

Description

The scroll functionality on the event list seems to be causing issues, hiding elements beneath the dashboard. This scrolling shouldn't exist.

jira issue

Solution description

We can change the LazyColumn to Column for the card layout, and we can use nested scroll view to make the events list and card layout scrollable.

Covered unit test cases

Describe the tests that you ran to verify your changes.

Where did you test this issue?

  • Smartphone Emulator
  • Tablet Emulator
  • Smartphone
  • Tablet

Which Android versions did you test this issue?

  • 4.4
  • 5.X - 6.X
  • 7.X
  • 8.X
  • 9.X - 10.X
  • 11.X - 13.X
  • Other

Checklist

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code

@msasikanth
Copy link
Contributor Author

I don't see the new card layout being used in landscape mode, and I couldn't trigger landscape view on my device. Should I change that as well?

@msasikanth msasikanth force-pushed the ANDROAPP-5663-tei-dashboard-event-list-scrolling branch 2 times, most recently from c6b5796 to 170e3cc Compare December 12, 2023 11:33
@msasikanth
Copy link
Contributor Author

Added support for landscape view as well

@ferdyrod
Copy link
Contributor

Hi @msasikanth

I just tested your PR, and everything is working great. The only thing that we want is that in Android 12+ there is a overscroll animation effect that we would like for it to disable it.

Once you have, let know to test again and proceed with merging you pr.

Thank you

@msasikanth
Copy link
Contributor Author

Hi @msasikanth

I just tested your PR, and everything is working great. The only thing that we want is that in Android 12+ there is a overscroll animation effect that we would like for it to disable it.

Once you have, let know to test again and proceed with merging you pr.

Thank you

Done. Pushed the changes

@msasikanth msasikanth force-pushed the ANDROAPP-5663-tei-dashboard-event-list-scrolling branch 3 times, most recently from 814d749 to 48fc86c Compare December 18, 2023 00:28
@andresmr andresmr force-pushed the ANDROAPP-5663-tei-dashboard-event-list-scrolling branch 3 times, most recently from 341b0d8 to 906e2ec Compare December 20, 2023 10:02
`LazyColumn` sets infinite height, if we have to place it inside another scroll view Compose will throw and runtime error saying that. We need to either set a fixed height to the `LazyColumn` or replace it with `Column` which wraps the content. Second one seemed like a good idea, so that it can react to content changes and recalculate the height.
We have to 2 options to make the entire content scroll in this screen.

1. We migrate the events items to use Compose as well and move them into `TeiDetailDashboard`. Which is ideal, but requires significant migration of UI and associated code with it. Since that's out of scope didn't go with that.

2. Since in earlier commit I have change the `LazyColumn` to `Column`, we can now place the card and the events list inside a nested scroll view to make the entire content scroll.

The 2nd option seems like a sensible change for now, until we migrate events list and eventually this entire screen.
@andresmr andresmr force-pushed the ANDROAPP-5663-tei-dashboard-event-list-scrolling branch from 906e2ec to 0de6b74 Compare December 20, 2023 14:47
Copy link

Quality Gate Passed Quality Gate passed

Kudos, no new issues were introduced!

0 New issues
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

@andresmr andresmr merged commit bcc17b2 into dhis2:develop Dec 22, 2023
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.

5 participants