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.

Sign up for GitHub

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

Jump to bottom

Feature: Add recent section, search bar and sorting to dashboard #384

Open
wants to merge 12 commits into
base: improvement/redesign-dashboard
Choose a base branch
from
Open

Feature: Add recent section, search bar and sorting to dashboard #384

wants to merge 12 commits into from

Conversation

julian-wls
Copy link
Contributor

@julian-wls julian-wls commented Feb 5, 2025 •
edited
Loading

Problem Description

As mentioned in #350 the recent section is currently missing in the Android app. Furthermore it is currently not possible to search for a course or change the ordering of courses in the last as it is in the web app.

Changes

This PR is a follow up PR of #380 and adds the recent section and a search bar as well as a button to change the alphabetical order of the list.
The recently accessed section includes the last three accessed courses and is is implemented in a new DashboardStorageService that uses androidx.datastore to store the last three accessed courses as a json string and provides decoding and encoding functionality.
The architecture of the dashboard state has been modified to be a mutableStateFlow to enable filtering and sorting.

Steps for testing

  1. After switching to this branch make sure there are no sections shown.
  2. Click on a course and go back to the dashboard and verify that the recently accessed section is now visible and shows this course.
  3. Click on three more courses and verify that the section never exceeds three courses and updates instantly after going back from a course.
  4. Search for a course and make sure everything works as expected.
  5. Change the sorting from alphabetically ascending to descending and vice versa, make sure that everything works as expected.

Screenshots

@julian-wls julian-wls self-assigned this Feb 5, 2025
@julian-wls julian-wls linked an issue Feb 6, 2025 that may be closed by this pull request
Redesign & restructure conversation settings #365
Open
@julian-wls julian-wls removed a link to an issue Feb 6, 2025
Redesign & restructure conversation settings #365
Open
@julian-wls julian-wls linked an issue Feb 6, 2025 that may be closed by this pull request
Redesign dashboard & introduce recent section #350
Open
@julian-wls julian-wls changed the title Feature: Add recent section to dashboard Feature: Add recent section, search bar and sorting to dashboard Feb 6, 2025
@julian-wls julian-wls added the ready for review This PR can be reviewed label Feb 6, 2025
@julian-wls julian-wls marked this pull request as ready for review February 6, 2025 14:43
FelberMartin
FelberMartin requested changes Feb 7, 2025
View reviewed changes
Copy link
Collaborator

@FelberMartin FelberMartin left a comment

Choose a reason for hiding this comment

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

Awesome, I like this change!

I have a few code comments and one general remark:
When using the sorting option or the search, it looks like the LazyColumn tries to remember which element the user has been looking at previously and adjusts the scroll state to still show that item after reordering.

https://github.com/user-attachments/assets/42d236b7-db20-4611-8495-c710fa6c53ff
Here the "General Testing Course" is the topmost course and in focus, and when I press the change order button, I would expect to see the (previous) bottommost element ("Test Course Matthias Linhuber"). But instead I still see the "General Testing Course" and have to scroll to the top of the list to see the "Test Course Matthias Linhuber"

...rc/main/kotlin/de/tum/informatics/www1/artemis/native_app/feature/dashboard/ui/CourseList.kt Outdated Show resolved Hide resolved
...n/de/tum/informatics/www1/artemis/native_app/feature/dashboard/ui/CourseOverviewViewModel.kt Outdated Show resolved Hide resolved
...n/de/tum/informatics/www1/artemis/native_app/feature/dashboard/ui/CourseOverviewViewModel.kt Show resolved Hide resolved
...n/de/tum/informatics/www1/artemis/native_app/feature/dashboard/ui/CourseOverviewViewModel.kt Outdated Show resolved Hide resolved
...in/kotlin/de/tum/informatics/www1/artemis/native_app/feature/dashboard/ui/CoursesOverview.kt Outdated Show resolved Hide resolved
...in/kotlin/de/tum/informatics/www1/artemis/native_app/feature/dashboard/ui/CoursesOverview.kt Outdated Show resolved Hide resolved
@julian-wls
Copy link
Contributor Author

I have a few code comments and one general remark: When using the sorting option or the search, it looks like the LazyColumn tries to remember which element the user has been looking at previously and adjusts the scroll state to still show that item after reordering.

https://github.com/user-attachments/assets/42d236b7-db20-4611-8495-c710fa6c53ff Here the "General Testing Course" is the topmost course and in focus, and when I press the change order button, I would expect to see the (previous) bottommost element ("Test Course Matthias Linhuber"). But instead I still see the "General Testing Course" and have to scroll to the top of the list to see the "Test Course Matthias Linhuber"

I agree, this is really unintuitive and feels strange, I added function to always save the last list position before changing the sorting and doing filtering. This way after doing a reordering the list can revert to its previous position. It should all work as expected now. Could you check again, @FelberMartin?

FelberMartin
FelberMartin approved these changes Feb 10, 2025
View reviewed changes
Copy link
Collaborator

@FelberMartin FelberMartin left a comment

Choose a reason for hiding this comment

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

Looking very nice now!

...rc/main/kotlin/de/tum/informatics/www1/artemis/native_app/feature/dashboard/ui/CourseList.kt Outdated Show resolved Hide resolved
eylulnc
eylulnc approved these changes Feb 11, 2025
View reviewed changes
Copy link
Contributor

@eylulnc eylulnc left a comment

Choose a reason for hiding this comment

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

Code lgtm!
Also functionality works as described 😊.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@FelberMartin FelberMartin FelberMartin approved these changes

@eylulnc eylulnc eylulnc approved these changes

Assignees

@julian-wls julian-wls

Labels
ready for review This PR can be reviewed
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Redesign dashboard & introduce recent section
3 participants
@julian-wls @FelberMartin @eylulnc