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

Add custom tag modal #1171

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Add custom tag modal #1171

wants to merge 2 commits into from

Conversation

revam
Copy link
Member

@revam revam commented Jan 20, 2025

I made sure the functionality of it is working as intended according to how I would assume it should work before I started this PR. As for all other aspects, feel free to roast it.

The final UI doesn't quite match the mock-ups, since I discovered while building/testing that a) the mock-up is old and the modal/dialog style is outdated compared to the current code base, and b) the lack of any indicator showing which tags are currently added/in-use, which I addressed, and c) the lack of any toggle for adding/removing tags from use on a series, which I also addressed.

Copy link
Member

@harshithmohan harshithmohan left a comment

Choose a reason for hiding this comment

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

The modal doesn't need to be stored in redux store if it's only being used from one place.

@revam revam force-pushed the add-custom-tag-modal branch from b6635ea to c861c19 Compare January 20, 2025 09:22
@revam revam force-pushed the add-custom-tag-modal branch from c861c19 to 511bf11 Compare January 20, 2025 09:24
@revam revam requested a review from harshithmohan January 20, 2025 11:02
Copy link
Member

@harshithmohan harshithmohan left a comment

Choose a reason for hiding this comment

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

Other than the comments. The flow is confusing.

Separate the add/edit functionality into a separate modal and keep this one for just selecting the tag. Maybe we should put the add/edit away in the settings or something. But right now it's just confusing. And it also needs to allow removing or adding multiple tags at once. It does do that.. I was expecting I just select things and click on "save"

Tagging @ElementalCrisis for thoughts

EDIT: Just realized this is EC's design..
image

src/core/react-query/tag/mutations.ts Outdated Show resolved Hide resolved
src/core/react-query/tag/queries.ts Outdated Show resolved Hide resolved
src/components/Collection/SeriesTopPanel.tsx Outdated Show resolved Hide resolved
src/components/Collection/SeriesTopPanel.tsx Outdated Show resolved Hide resolved
src/components/Collection/SeriesTopPanel.tsx Outdated Show resolved Hide resolved
@ElementalCrisis
Copy link
Member

Other than the comments. The flow is confusing.

Separate the add/edit functionality into a separate modal and keep this one for just selecting the tag. Maybe we should put the add/edit away in the settings or something. But right now it's just confusing. And it also needs to allow removing or adding multiple tags at once. It does do that.. I was expecting I just select things and click on "save"

Tagging @ElementalCrisis for thoughts

EDIT: Just realized this is EC's design.. image

Tags in general need a better way to view/manage, being tied strictly to shows or applied in a filter leaves a lot to be desired. An idea I have is more global tag management where you can search/view tags, see how many series have them, easily manage custom tags, hide/ignore tags.

Something we can look into for V3 if there is demand and want for it.

@revam revam requested a review from harshithmohan February 4, 2025 15:42
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.

3 participants