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

Added ability to favorite communities in drawer #971

Merged
merged 10 commits into from
Dec 15, 2023

Conversation

hjiangsu
Copy link
Member

@hjiangsu hjiangsu commented Dec 8, 2023

Pull Request Description

This PR adds the ability to favorite subscribed communities. When a community is favorited, it will show up in a new section on the drawer for easier access.

Some notes:

  • Refactored the drawer to be more modular, and to follow existing conventions better
  • Fixed the issue where tapping on the user indicator in the drawer did not navigate to the user page
  • Favourite communities are only available when the user is logged in. It is disabled for anonymous users for the time being
  • I opted to use the database rather than SharedPreferences since the data is slightly more complex (account id, community id). The downside here is that we don't have a method to restore or backup database files, so favourites would be lost on re-installs.

Some more testing would be ideal for the database migrations (adding in a new table for favorites) from 0.2.6 -> 0.2.7, and also for newly created databases.

Issue Being Fixed

Issue Number: #526

Screenshots / Recordings

9c780975-1945-4d4f-8f18-622d8deae7ae.mp4

Checklist

  • Did you update CHANGELOG.md?
  • Did you use localized strings where applicable?
  • Did you add semanticLabels where applicable for accessibility?

@hjiangsu hjiangsu marked this pull request as ready for review December 12, 2023 20:27
@hjiangsu
Copy link
Member Author

@micahmo Feel free to test this out - especially for database migrations to make sure things go smoothly from last version to this version

Copy link
Member

@micahmo micahmo left a comment

Choose a reason for hiding this comment

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

Awesome feature! The code looks good!

I have a few overall comments.

  • Does this work with anonymous subscriptions?
  • There's a funny UI thing where an inkwell effect occurs on a different community after favoriting. I've seen this before and I think it might be a weirdness of Flutter. Something to look into later.
  • I am currently unable to test this feature because I get the following error. I think maybe I grabbed an earlier version of this branch before you had finalized the db changes. If I just need to delete me db, let me know!
    E/SQLiteLog(19102): (1) table favorites already exists in "CREATE TABLE favorites(id STRING PRIMARY KEY, accountId STRING, communityId INTEGER)"
    I/flutter (19102): error DatabaseException(table favorites already exists (code 1 SQLITE_ERROR): , while compiling: CREATE TABLE favorites(id STRING PRIMARY KEY, accountId STRING, communityId INTEGER)) sql 'CREATE TABLE favorites(id STRING PRIMARY KEY, accountId STRING, communityId INTEGER)' args [] during open, closing...
    

I'm excited about the possibility of utilizing favorites in other parts of the app (e.g., showing in search, prioritizing in autocomplete list, etc., etc.).

@hjiangsu
Copy link
Member Author

Does this work with anonymous subscriptions?

Favourite communities are only available when the user is logged in. It is disabled for anonymous users for the time being

I mentioned it on the PR description but nope, this only works with logged in users for now! I would need to add in some more logic to make it work with anonymous accounts, which could be an improvement for the future.

There's a funny UI thing where an inkwell effect occurs on a different community after favoriting.

Interesting, I'll take a look at this! I didn't notice it but I'll see if I can reproduce it

I am currently unable to test this feature because I get the following error.

Yup, I think that might be the case, I did some db changes in between so it might've messed things up. I think clearing db and relaunching it should do the trick!

@micahmo
Copy link
Member

micahmo commented Dec 13, 2023

I mentioned it on the PR description but nope, this only works with logged in users for now! I would need to add in some more logic to make it work with anonymous accounts, which could be an improvement for the future.

Sorry I missed that! Makes sense though!

Interesting, I'll take a look at this! I didn't notice it but I'll see if I can reproduce it

I can see it in your video! Look at the first time you favorite a community. After you press the star, the star next to the top unfavorited community blinks. Again it's something I've seen with Flutter before. I think it's related to it getting confused about which widget was activated after the UI rebuilds.

Yup, I think that might be the case, I did some db changes in between so it might've messed things up. I think clearing db and relaunching it should do the trick!

Will do!

@micahmo
Copy link
Member

micahmo commented Dec 13, 2023

I think clearing db and relaunching it should do the trick!

This works fine once, but it looks like we can't switch to an older version and switch back. I don't know if there's a way to conditionally create the table if it doesn't exist. If not, we'll just have to know that we don't support downgrading from and then upgrading back to this version.

Otherwise the feature works great! 😊

@hjiangsu
Copy link
Member Author

After you press the star, the star next to the top unfavorited community blinks.

Ahh, I wonder if this is just due to the animation delay of tapping the icon! Regardless, I think this is a minor thing and shouldn't be too much of an issue

I don't know if there's a way to conditionally create the table if it doesn't exist.

I can do a try-catch on the creation logic so that it doesn't hang there! I think that should generally work in the cases where the table schema is left untouched (e.g., we don't change the schema between versions)

@hjiangsu
Copy link
Member Author

This should be good now! I added try-catch when creating or upgrading the database. Feel free to give it a quick test and let me know if it works!

Note that this only catches the error and proceeds forward. It doesn't attempt to fix any issues that it catches. This should allow the user to at least load Thunder, and delete the database from the Settings page to reset things back to default if needed!

Copy link
Member

@micahmo micahmo left a comment

Choose a reason for hiding this comment

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

LGTM!

@hjiangsu hjiangsu merged commit 2603ba7 into develop Dec 15, 2023
1 check passed
@hjiangsu hjiangsu deleted the feature/favorite-subscriptions branch December 15, 2023 01:06
@micahmo
Copy link
Member

micahmo commented Dec 15, 2023

I just noticed that we lost the full community name on the second line of the listings in the drawer, so we can only see the display name by default. Was that intentional?

@hjiangsu
Copy link
Member Author

Was that intentional?

It was intentional! I was playing around with the idea of hiding the full community name in the drawer now that we have a favorites action.

Because of the new action, the amount of horizontal real estate we have to display information is reduced. So to compensate for that, I thought I'd remove the full community name since it feels a bit redundant to have (we already have the community name). To add on to that, we can always see the full community name through the tooltip if its needed! Do you have any thoughts on this change?

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.

2 participants