-
-
Notifications
You must be signed in to change notification settings - Fork 455
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
feat: add hideTags option in user settings #3620
Conversation
src/hooks/useUser.ts
Outdated
export interface UserSettings { | ||
discordId?: string; | ||
region?: string; | ||
originalLanguage?: string; | ||
locale?: string; | ||
type UserSettings = Pick< | ||
UserSettingsGeneralResponse, | ||
'discordId' | 'region' | 'originalLanguage' | 'locale' | 'hideTags' | ||
> & { | ||
notificationTypes: Partial<NotificationAgentTypes>; | ||
watchlistSyncMovies?: boolean; | ||
watchlistSyncTv?: boolean; | ||
} | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I noticed here that we're duplicating code and having multiple sources of truth. This Pick type allows us to honor the types already set for these settings in the UserSettingsGeneralResponse interface. If it's desired to have it typed out explicitly in this file though, we can simply remove what I added and instead add hideTags?: boolean;
underneath line 32 of the original code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we should modify any older code unless necessary. The hideTags boolean should suffice.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good, I'll remove this chunk then!
@OwsleyJr do you know how I can get help with data migration stuff? My understanding is pretty limited in that side of things, and there seems to be some SQL build errors in Cypress like:
|
This new option will allow for a user to hide the tags that show in a Movie or TV Show details page.
e436449
to
918618f
Compare
Would just need to generate a migration file. You can look in the package.json for the script we use. |
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
Description
This new option will allow for a user to hide the tags that show in a Movie or TV Show details page. If the setting
Hide Tags
is checked, the tags will not show. If it is unchecked, the tags will show.Screenshot (if UI-related)
Setting:
Before:
After:
To-Dos
yarn build
yarn i18n:extract
Issues Fixed or Closed