Move useLoadMore functionality in the Load More component #1238

obulat · 2022-04-05T15:33:03Z

Fixes

Fixes #1235 by @obulat
Fixes #1301 by @sarayourfriend
Replacement of #1236

Description

This PR moves all the logic for VLoadMore button to the VLoadMore component, and heavily uses the data available in the media store.
This solves the problem where because of an error in VLoadMore, there was a server-client mismatch, and the whole app was re-rendering, and was not interactive on SSR-ed search results.

This branch is based on type_base_components because it uses TS and VButton. It is, therefore, blocked by #1233.

Testing Instructions

Go to localhost:8443/search/audio/?q=galah
You should see a single result, but other than that, the page should be interactive (search type button in the header should work), and the waveform should be displayed.
Compare it with the behavior on main by going to https://search-staging/search/audio/?q=galah

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.

sarayourfriend · 2022-04-06T07:44:00Z

For me when I go to http://192.168.1.105:8443/search/audio?q=galah it also shows a "load more" button. When I click it the page switches to the "no results" page 🤔

sarayourfriend

Also noticed that if you enable a file extension filter that results in 0 results (like mp3 for galah audio search) and then toggle it back off, it still shows no results.

obulat · 2022-04-07T04:02:47Z

The reason for failures described here is the fact that instead of using the store state from the server, we are re-initializing it on the client, for all the Pinia stores.
This means that even though we got results on the server, the result counts on the client are set to 0 in the beginning. FetchState is in the initial state, and page is set to undefined, so you can click on the Load More button.

# Conflicts: # src/composables/use-fetch-state.ts # src/locales/po-files/openverse.pot # test/unit/specs/components/related-images.spec.js

sarayourfriend

This is already an improvement as the page doesn't crash, at least, but there is still something wrong with the load more logic.

When I visit http://localhost:8443/search/audio?q=galah on this branch the page doesn't crash and remains interactive. However there is a load more button (even though there's only a single result). If I click it, then a "no results" page will show.

sarayourfriend · 2022-04-13T06:38:36Z

src/components/VLoadMore.vue

    }
+    const isFetching = computed(() => mediaStore.fetchState.isFetching)
+
+    const buttonLabel = computed(() => i18n.t('browse-page.load'))


I see that it was already this way, but why does this need to be a computed? 🤔

No it doesn't 😆 I think I wrote it that way when I was only starting to learn the way Composition API worked.

+1 to putting this directly on the template 🙂

sarayourfriend · 2022-04-13T06:48:31Z

test/unit/specs/components/image-grid.spec.js

+  it('renders images without load more button', () => {
    render(VImageGrid, options)
    expect(screen.queryAllByRole('img').length).toEqual(propsData.images.length)
    expect(screen.queryAllByRole('figure').length).toEqual(
      propsData.images.length
    )
-    const loadMoreButton = screen.queryByTestId('load-more')
-    expect(loadMoreButton).toBeVisible()
-    expect(loadMoreButton).toHaveTextContent(messages['browse-page'].load)
+    expect(screen.queryByTestId('load-more')).not.toBeVisible()


This test file now misses the other side of it, when the load more button does show.

Also I think there are three branches to test for when load more shows or not, ideally we'd test those in a VLoadMore specific test file:

searchStore.searchTerm !== '' && mediaStore.fetchState.canFetch && mediaStore.resultCount > 0

Each of those is a distinct branch we'd want to test 😕

Then the image-grid and audio-grid tests can just confirm that the load more component is actually rendered (whether the button displays or not is now irrelevant to the grid as it's handled by the button itself, thank goodness!)

sarayourfriend · 2022-04-13T06:50:54Z

src/components/VLoadMore.vue

    :disabled="isFetching"
+    data-testid="load-more"


Is this necessary or could we test just using the label? Generally it's preferable to avoid introducing test id's if possible, in this case a getByText should be okay, I think.

obulat · 2022-04-14T07:09:41Z

This is already an improvement as the page doesn't crash, at least, but there is still something wrong with the load more logic.

When I visit http://localhost:8443/search/audio?q=galah on this branch the page doesn't crash and remains interactive. However there is a load more button (even though there's only a single result). If I click it, then a "no results" page will show.

Sorry for not stating it clearly, but this PR should only work after the media PR is merged.

# Conflicts: # src/locales/po-files/openverse.pot

sarayourfriend

LGTM! Massive improvement 🎉

krysal

LGTM! It would just be nice to cover more cases in the tests in the future as @sarayourfriend mentioned, but it's nice to fix this obvious bug!

obulat requested a review from a team as a code owner April 5, 2022 15:33

obulat requested review from krysal and dhruvkb April 5, 2022 15:33

openverse-bot added the 🚦 status: awaiting triage Has not been triaged & therefore, not ready for work label Apr 5, 2022

obulat force-pushed the really_fix_load_more branch from 3d37398 to 77df68b Compare April 5, 2022 15:46

obulat mentioned this pull request Apr 5, 2022

Fix useLoadMore composable #1236

Closed

7 tasks

sarayourfriend reviewed Apr 6, 2022

View reviewed changes

Base automatically changed from type_base_components to main April 7, 2022 03:09

obulat force-pushed the really_fix_load_more branch from 88820ea to 446a130 Compare April 7, 2022 04:20

obulat marked this pull request as draft April 11, 2022 16:25

obulat changed the title ~~Refactor Load More button to fix interactivity on search results with fewer than 20 results~~ Move useLoadMore functionality in the Load More component Apr 12, 2022

obulat added 2 commits April 12, 2022 19:49

Working state hydration

f9647fb

Leave only the VLoadMore changes

a6e3cad

obulat added 🟧 priority: high Stalls work on the project or its dependents and removed 🟥 priority: critical Must be addressed ASAP labels Apr 12, 2022

obulat marked this pull request as ready for review April 13, 2022 03:28

Remove unused VLoadMore props in the components

86f01f7

obulat force-pushed the really_fix_load_more branch from 446a130 to 86f01f7 Compare April 13, 2022 03:38

obulat added 3 commits April 13, 2022 19:38

Add pot file

e8f33b7

Merge branch 'main' into really_fix_load_more

4d82e1b

# Conflicts: # src/composables/use-fetch-state.ts # src/locales/po-files/openverse.pot # test/unit/specs/components/related-images.spec.js

Update tests and pot file

a1a633b

sarayourfriend reviewed Apr 14, 2022

View reviewed changes

obulat marked this pull request as draft April 14, 2022 07:10

Merge branch 'main' into really_fix_load_more

486856c

# Conflicts: # src/locales/po-files/openverse.pot

obulat mentioned this pull request Apr 20, 2022

If there are 0 audio results on the All Content view (but some images), the Load more button is not clickable #1301

Closed

1 task

obulat marked this pull request as ready for review April 20, 2022 16:54

Merge branch 'main' into really_fix_load_more

f37450a

# Conflicts: # src/locales/po-files/openverse.pot

sarayourfriend approved these changes Apr 25, 2022

View reviewed changes

obulat enabled auto-merge (squash) April 25, 2022 14:52

obulat added this to the v3.2.1 - Must haves milestone May 2, 2022

krysal approved these changes May 2, 2022

View reviewed changes

obulat merged commit a6d5b05 into main May 2, 2022

obulat deleted the really_fix_load_more branch May 2, 2022 17:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move useLoadMore functionality in the Load More component #1238

Move useLoadMore functionality in the Load More component #1238

obulat commented Apr 5, 2022 •

edited

Loading

sarayourfriend commented Apr 6, 2022

sarayourfriend left a comment

obulat commented Apr 7, 2022

sarayourfriend left a comment

sarayourfriend Apr 13, 2022

obulat Apr 14, 2022

krysal May 2, 2022

sarayourfriend Apr 13, 2022

sarayourfriend Apr 13, 2022

obulat commented Apr 14, 2022

sarayourfriend left a comment

krysal left a comment

Move useLoadMore functionality in the Load More component #1238

Move useLoadMore functionality in the Load More component #1238

Conversation

obulat commented Apr 5, 2022 • edited Loading

Fixes

Description

Testing Instructions

Checklist

Developer Certificate of Origin

sarayourfriend commented Apr 6, 2022

sarayourfriend left a comment

Choose a reason for hiding this comment

obulat commented Apr 7, 2022

sarayourfriend left a comment

Choose a reason for hiding this comment

sarayourfriend Apr 13, 2022

Choose a reason for hiding this comment

obulat Apr 14, 2022

Choose a reason for hiding this comment

krysal May 2, 2022

Choose a reason for hiding this comment

sarayourfriend Apr 13, 2022

Choose a reason for hiding this comment

sarayourfriend Apr 13, 2022

Choose a reason for hiding this comment

obulat commented Apr 14, 2022

sarayourfriend left a comment

Choose a reason for hiding this comment

krysal left a comment

Choose a reason for hiding this comment

obulat commented Apr 5, 2022 •

edited

Loading