Convert search store to an options store #1259

obulat · 2022-04-12T04:51:17Z

Fixes

Description

This PR converts the setup Pinia store into options store. This means that we pass an options object to the defineStore function that has state, getters and actions, similar to Vuex stores.

This PR also resets the state when homepage mounts so that the old search type is not used when navigating back to the homepage from a search page using the logo, and searching again.

Testing Instructions

Try searching for all content, images and audios, opening single results, searching from the homepage

Checklist

My pull request has a descriptive title (not a vague title like Update index.md).
My pull request targets the default branch of the repository (main) or a parent feature branch.
My commit messages follow best practices.
My code follows the established code style of the repository.
I added or updated tests for the changes I made (if applicable).
I added or updated documentation (if applicable).
I tried running the project locally and verified that there are no visible errors.

Developer Certificate of Origin

Developer Certificate of Origin
Version 1.1

Copyright (C) 2004, 2006 The Linux Foundation and its contributors.
1 Letterman Drive
Suite D4700
San Francisco, CA, 94129

Everyone is permitted to copy and distribute verbatim copies of this
license document, but changing it is not allowed.


Developer's Certificate of Origin 1.1

By making a contribution to this project, I certify that:

(a) The contribution was created in whole or in part by me and I
    have the right to submit it under the open source license
    indicated in the file; or

(b) The contribution is based upon previous work that, to the best
    of my knowledge, is covered under an appropriate open source
    license and I have the right under that license to submit that
    work with modifications, whether created in whole or in part
    by me, under the same open source license (unless I am
    permitted to submit under a different license), as indicated
    in the file; or

(c) The contribution was provided directly to me by some other
    person who certified (a), (b) or (c) and I have not modified
    it.

(d) I understand and agree that this project and the contribution
    are public and that a record of the contribution (including all
    personal information I submit with it, including my sign-off) is
    maintained indefinitely and may be redistributed consistent with
    this project or the open source license(s) involved.

zackkrida

From a UI perspective, everything I tested is working correctly:

Toggling and clearing filters
SSR and client-side page loads
Switching content types

zackkrida · 2022-04-12T15:22:33Z

src/pages/index.vue

+    onMounted(() => {
+      searchStore.$reset()
+    })


So this clears all the search information, but it doesn't clear the results. So given the following flow:

Search for 'dogs' on the homepage

See the results on the /search page

Click the openverse logo / home link

Make a new search for 'cats'

The user will see a flash of dog results while the 'cat' results are loading.

Not sure if that's a problem or should be fixed here, just noting.

Ah, thank you for noting this! I'll add add the result reset, too

Oh, wait. This will require the reset in the media store. Let me make an issue for fixing it after one of the store PRs is merged.

sarayourfriend

I found a few bugs testing this.

#1262
#1263
#1264
#1265

None of them are caused by this branch though 🎉

Just a couple questions about the tests but should be able to approve after that 😁

src/stores/search.ts

sarayourfriend · 2022-04-13T08:43:50Z

src/stores/search.ts

+      return mediaFilterKeys[state.searchType]
+        .filter((filterKey) => filterKey !== 'mature')
+        .reduce((obj, filterKey) => {
+          obj[filterKey] = this.filters[filterKey]


Should we choose to use either this or the explicit state parameter consistently? Mixing the two is confusing.

We have to use this for some getters because we are calling actions from within them. Using state as a param is convenient because you don't have to explicitly add the return type of the getter. But it using both is confusing, then we should just stick to this, and manually typing the return type.
I'll refactor all the getters if we agree that it's better for consistency, @sarayourfriend

Ah interesting. I guess it's fine to mix them, I will get used to it!

test/unit/specs/stores/search-store.spec.js

krysal

Everything seems to work as expected 👍

Co-authored-by: sarayourfriend <24264157+sarayourfriend@users.noreply.github.com>

Convert search store to an options store

16b633c

obulat requested a review from a team as a code owner April 12, 2022 04:51

obulat requested review from krysal and sarayourfriend April 12, 2022 04:51

openverse-bot added 💻 aspect: code Concerns the software code in the repository 🟧 priority: high Stalls work on the project or its dependents 🧰 goal: internal improvement Improvement that benefits maintainers, not users labels Apr 12, 2022

zackkrida approved these changes Apr 12, 2022

View reviewed changes

zackkrida reviewed Apr 12, 2022

View reviewed changes

obulat mentioned this pull request Apr 12, 2022

Home page should reset the media results on mounted #1261

Closed

1 task

sarayourfriend reviewed Apr 13, 2022

View reviewed changes

krysal approved these changes Apr 13, 2022

View reviewed changes

obulat and others added 2 commits April 14, 2022 06:39

Update src/stores/search.ts

a2fa74c

Co-authored-by: sarayourfriend <24264157+sarayourfriend@users.noreply.github.com>

Merge branch 'main' into pinia_search_options_store

cf95400

obulat merged commit b9f3d99 into main Apr 14, 2022

obulat deleted the pinia_search_options_store branch April 14, 2022 04:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Convert search store to an options store #1259

Convert search store to an options store #1259

obulat commented Apr 12, 2022 •

edited

Loading

zackkrida left a comment

zackkrida Apr 12, 2022

obulat Apr 12, 2022

obulat Apr 12, 2022

sarayourfriend left a comment

sarayourfriend Apr 13, 2022

obulat Apr 13, 2022

sarayourfriend Apr 13, 2022

krysal left a comment

Convert search store to an options store #1259

Convert search store to an options store #1259

Conversation

obulat commented Apr 12, 2022 • edited Loading

Fixes

Description

Testing Instructions

Checklist

Developer Certificate of Origin

zackkrida left a comment

Choose a reason for hiding this comment

zackkrida Apr 12, 2022

Choose a reason for hiding this comment

obulat Apr 12, 2022

Choose a reason for hiding this comment

obulat Apr 12, 2022

Choose a reason for hiding this comment

sarayourfriend left a comment

Choose a reason for hiding this comment

sarayourfriend Apr 13, 2022

Choose a reason for hiding this comment

obulat Apr 13, 2022

Choose a reason for hiding this comment

sarayourfriend Apr 13, 2022

Choose a reason for hiding this comment

krysal left a comment

Choose a reason for hiding this comment

obulat commented Apr 12, 2022 •

edited

Loading