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

Feat/identify items Add identify Items component to item details page #114

Closed
wants to merge 2 commits into from

Conversation

camc314
Copy link
Contributor

@camc314 camc314 commented Sep 24, 2020

No description provided.

@heyhippari
Copy link
Contributor

I'm not a fan of this... :/ Some issues to be addressed:

  • We don't want dialogs to be added to every page that need them. I've been trying to figure out a good way to do this for a few days. Right now I'm edging towards something like portal-vue, but I also really like the idea behind vuetify-modal.
  • Dialogs should be a lot more modular. Ideally, we want very basic Dialog component, which would be just a v-dialog with some common props and sensible defaults. Then we want a few specific but still generic dialogs (not necessarily in this PR). For this, we probably want to go Base Dialog -> Card Dialog (more on this later) -> Identify Dialog
  • The item menu is a generic component. It should be accessible from the cards as well. feat(card): add metadata editor #68 has a very rough first implementation of one. A good way to make it usable everywhere is to expose the activator slot in it, then we can use that to define the button we want. Or maybe just set a "card" prop that makes the activator the one in feat(card): add metadata editor #68 while the other is the "more" button.
  • The dialog itself is pretty ugly (not sure if it's finished though, but I wanted to get feedback out early). We should use a card inside of the v-dialog, with a title area and a bottom area for buttons. This is what I meant by "Card Dialog" above. I'd expose two slots ("Buttons" and "Content"), as well as a prop for the title. We can add more later if we need to.
  • It's missing a ton of search options. Year and Ids are all missing. Path also isn't shown, which can be useful for the user to be able to see which item they're actually trying to identify. We can probably do Ids later, but year should be straightforward enough to be quick to do.
  • I'm not quite sure if we need "Replace all images" or not.
  • The dialog itself should be bigger. We should probably have a few preset sizes on the Base Dialog, set via props. Then various types of dialogs can request a specific size.
  • "No items found" shouldn't be shown if no search has been performed. It's kind of confusing for users and doesn't look nice.

I think adopting the Plex terminology of "Fix match" would also make transition easier for users. It fits just as well as Identify (perhaps more, as the server will already match stuff, you're only telling it that it's wrong). It's debatable though. Probably ask @jellyfin/core what they feel fits better for the term to use.

Separating the results and the search fields in two screens could work as well, especially with the number of fields we have. It's how both Plex and Emby do it. Not that we have to copy them, it's good to rethink existing things as we go along, but I think it makes sense here.

@heyhippari heyhippari mentioned this pull request Sep 25, 2020
90 tasks
@sonarcloud
Copy link

sonarcloud bot commented Sep 25, 2020

Kudos, SonarCloud Quality Gate passed!

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities (and Security Hotspot 0 Security Hotspots to review)
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@camc314
Copy link
Contributor Author

camc314 commented Sep 25, 2020

We don't want dialogs to be added to every page that need them. I've been trying to figure out a good way to do this for a few days. Right now I'm edging towards something like portal-vue, but I also really like the idea behind vuetify-modal.

Would infomation be passed to a store and then opened using portal-vue/vuetify-moday?

  • The dialog itself is pretty ugly (not sure if it's finished though, but I wanted to get feedback out early). We should use a card inside of the v-dialog, with a title area and a bottom area for buttons. This is what I meant by "Card Dialog" above. I'd expose two slots ("Buttons" and "Content"), as well as a prop for the title. We can add more later if we need to.

That's true, I've reworked it. I think it looks a lot better now. I haven't used the card component but the v-stepper. (one stage for searching, identifying, and confirming). Let me know what you think.

It's missing a ton of search options. Year and Ids are all missing. Path also isn't shown, which can be useful for the user to be able to see which item they're actually trying to identify. We can probably do Ids later, but year should be straightforward enough to be quick to do.

I've added searching by item year, add the file name. TMDB/IMDb Ids still need to be added

I'm not quite sure if we need "Replace all images" or not.

This is included in JF-Web and is an option in the API. From the testing I have done, if it is not selected, it is much quicker to identify and save to the DB, since the server doesn't have to collect metadata before responding to the request.

"No items found" shouldn't be shown if no search has been performed. It's kind of confusing for users and doesn't look nice.

This should be fixed now.

@heyhippari
Copy link
Contributor

Would infomation be passed to a store and then opened using portal-vue/vuetify-moday?

Thinking about it, we might need a "ModalManager" component, which uses a Vuex store to show and hide various modals, and set their contents (possibly via calling actions on other stores).
I don't think vue-portals is really needed here. The component would be on a handful of layouts, so it's not really that duplicated.

That's true, I've reworked it. I think it looks a lot better now. I haven't used the card component but the v-stepper. (one stage for searching, identifying, and confirming). Let me know what you think.

Looks much better :)

@camc314 camc314 mentioned this pull request Dec 3, 2020
@heyhippari heyhippari added this to the Preview Release 1 milestone Dec 6, 2020
@camc314 camc314 force-pushed the feat/identifyItems branch 2 times, most recently from 53fefba to 4ec0233 Compare December 15, 2020 08:50
@sonarcloud
Copy link

sonarcloud bot commented Dec 22, 2020

Kudos, SonarCloud Quality Gate passed!

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@heyhippari
Copy link
Contributor

@camc314 Is this ready for review?

@camc314
Copy link
Contributor Author

camc314 commented Dec 26, 2020

@camc314 Is this ready for review?

Not quite, I've got to fix the card width when selecting an item

@sonarcloud
Copy link

sonarcloud bot commented Feb 15, 2021

Kudos, SonarCloud Quality Gate passed!

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@camc314 camc314 closed this Mar 13, 2021
@camc314 camc314 deleted the feat/identifyItems branch March 13, 2021 12:14
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