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(search): add search #345

Merged
merged 11 commits into from
Jul 13, 2021
Merged

feat(search): add search #345

merged 11 commits into from
Jul 13, 2021

Conversation

heyhippari
Copy link
Contributor

@heyhippari heyhippari commented Dec 6, 2020

It searches.

image

@sonarcloud
Copy link

sonarcloud bot commented Dec 6, 2020

Kudos, SonarCloud Quality Gate passed!

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

No Coverage information No Coverage information
0.0% 0.0% Duplication

@heyhippari heyhippari marked this pull request as draft December 7, 2020 13:40
@heyhippari
Copy link
Contributor Author

Converted back to draft, as it needs additional UI design, since I'm not happy with the way this looks.

@ferferga
Copy link
Member

ferferga commented Dec 9, 2020

This probably should be in it's own discussion thread but:

What about renaming "People" tab to whatever category those "People" fall in? Just like current web, where they appear as "Artists" and "People".

Imo, we should have three categories:

  • Artists
  • Cast & Crew
  • People

Although we don't have this feature yet, we shouldn't mix people's faces from photo libraries with the ones of movies/music/TV Shows.

I don't know how this is currently implemented but we don't have to add tabs when there are no items that match the query, so this shouldn't increase the UI's weight.

Again, this is completely unnecessary to do right now, but I think it's better if we do it this way since the beginning, instead of getting users "accostumed" to it later.

@Nickbert7
Copy link

I don't know if it is difficult to add, but would it be possible also include tags into the search?

@codecov-io
Copy link

codecov-io commented Jan 16, 2021

Codecov Report

Merging #345 (91c231d) into master (9ad6a54) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #345   +/-   ##
=======================================
  Coverage   53.41%   53.41%           
=======================================
  Files          24       24           
  Lines         863      863           
  Branches      151      151           
=======================================
  Hits          461      461           
  Misses        390      390           
  Partials       12       12           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9ad6a54...91c231d. Read the comment docs.

pages/search.vue Outdated Show resolved Hide resolved
store/search.ts Outdated Show resolved Hide resolved
@ferferga
Copy link
Member

Maybe we should replace the skeleton loader while searching with a spinning circle? There's a lot of jumpiness when searching for music, for instance.

@heyhippari
Copy link
Contributor Author

Maybe we should replace the skeleton loader while searching with a spinning circle? There's a lot of jumpiness when searching for music, for instance.

I'm thinking about getting rid of the loader entirely, actually.

I think we should be able to combine asyncData and fetch to handle fetching information, as well as debouncing the search input to prevent it from triggering a search request on each input change.

This should make the experience a little better.

@ferferga
Copy link
Member

ferferga commented Feb 10, 2021

@camc314 In case you want to take over this PR, I remember MrTimscampi mentioned that he wanted to follow this design for this: #522

If not, I can take a look at it this weekend maybe.

@camc314
Copy link
Contributor

camc314 commented Feb 10, 2021

@camc314 In case you want to take over this PR, I remember MrTimscampi mentioned that he wanted to follow this design for this: #522

If not, I can take a look at it this weekend maybe.

Ah that design there looks good. All I have done, is rebased and moved some of the type fixings to a separate PR

@camc314 camc314 force-pushed the search branch 4 times, most recently from 7545d64 to 7692660 Compare February 11, 2021 19:33
@camc314
Copy link
Contributor

camc314 commented Feb 11, 2021

@ferferga I've got it to a good enough state to be reviewed, any improvements based on the discussion can be done at a later date imo.

@ThibaultNocchi
Copy link
Member

ThibaultNocchi commented Feb 12, 2021

@camc314 I got errors loading the home page, I'm trying to see what it is

TypeError: String.prototype.search called on incompatible undefined
    _render vue-i18n.esm.js:1605
    _interpolate vue-i18n.esm.js:1511
    _translate vue-i18n.esm.js:1731
    _t vue-i18n.esm.js:1756
    $t vue-i18n.esm.js:233
    render default.vue:188
    VueJS 30
    NuxtJS 4
    Babel 18
    NuxtJS 2
    Babel 12
    NuxtJS 11
client.js:97
    _callee$ NuxtJS
    Babel 8
    VueJS 32
    NuxtJS 4
    Babel 18
    NuxtJS 2
    Babel 12
    NuxtJS 11
TypeError: String.prototype.search called on incompatible undefined
    _render vue-i18n.esm.js:1605
    _interpolate vue-i18n.esm.js:1511
    _translate vue-i18n.esm.js:1731
    _t vue-i18n.esm.js:1756
    $t vue-i18n.esm.js:233
    render default.vue:188
    VueJS 13
    SET_USER_VIEWS userViews.ts:36
    wrappedMutationHandler vuex.esm.js:844
    commitIterator vuex.esm.js:466
    commit vuex.esm.js:465
    _withCommit vuex.esm.js:624
    commit vuex.esm.js:464
    boundCommit vuex.esm.js:409
    commit vuex.esm.js:796
    _callee$ userViews.ts:52
    Babel 10
    wrappedActionHandler vuex.esm.js:851
    dispatch vuex.esm.js:516
    boundDispatch vuex.esm.js:406
    dispatch vuex.esm.js:779
    mappedAction vuex.esm.js:1064
    beforeMount default.vue:183
    VueJS 18
client.js:97

Edit: lmao he doesn't like the localization in the placeholder, which is already similarly used in master

Edit 2: he doesn't like search.search but accepts save for instance

Edit 3: guess what, it's because I'm in French and in the locale file there's no search object, only the current string. So ofc it crashes trying to access a property of a string :p

@@ -208,7 +209,10 @@
"role": "Role",
"save": "Save",
"saved": "Saved",
"search": "Search",
"search": {
"search": "search",
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
"search": "search",
"search": "Search",

IMO, it's weird not having this capitalized

@ThibaultNocchi
Copy link
Member

So I pushed the needed changes for the locales, we'll have to check before merging if no other new "search" strings were added and add them to our PR if it is

@camc314
Copy link
Contributor

camc314 commented Feb 12, 2021

I don't think we should be changing strings in other locale files. It should be done on weblate. In prod, it shouldn't cause an issue, just produce an error in the console

Copy link
Member

@ferferga ferferga left a comment

Choose a reason for hiding this comment

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

Search doesn't work outside the main page, so you can't search if you're browsing a library, for instance.

client/layouts/default.vue Outdated Show resolved Hide resolved
@jellyfin-bot jellyfin-bot added the merge conflict Something has merge conflicts label Jun 1, 2021
@heyhippari heyhippari requested a review from ferferga June 3, 2021 14:06
@jellyfin-bot jellyfin-bot removed the merge conflict Something has merge conflicts label Jun 3, 2021
@sonarcloud
Copy link

sonarcloud bot commented Jun 3, 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

@heyhippari heyhippari dismissed ferferga’s stale review July 13, 2021 08:23

No longer relevant, prevents merging

Ongoing development automation moved this from In progress to Reviewer approved Jul 13, 2021
@heyhippari
Copy link
Contributor Author

@jellyfin-bot rebase

@jellyfin-bot jellyfin-bot moved this from Reviewer approved to In progress in Ongoing development Jul 13, 2021
@sonarcloud
Copy link

sonarcloud bot commented Jul 13, 2021

Kudos, SonarCloud Quality Gate passed!

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

No Coverage information No Coverage information
0.0% 0.0% Duplication

@heyhippari heyhippari merged commit 9eaffe5 into master Jul 13, 2021
Ongoing development automation moved this from In progress to Done Jul 13, 2021
@heyhippari heyhippari deleted the search branch July 13, 2021 08:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
vue Pull requests that edit or add Vue files
Projects
Development

Successfully merging this pull request may close these issues.

None yet

10 participants