DataView: only reset page if search has actually changed #61307
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
|
Size Change: +169 B (+0.01%) Total Size: 1.74 MB
ℹ️ View Unchanged
|
|
Flaky tests detected in ed2d50a. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/8919709370
|
arthur791004
reviewed
May 2, 2024
| page: 1, | ||
| search: debouncedSearch, | ||
| } ); | ||
| if ( debouncedSearch !== view.search ) { |
There was a problem hiding this comment.
Do we need to add view to the deps array?
There was a problem hiding this comment.
In the demo you shared, pagination is reset after layout switch. I don't know how you're using DataViews in your code, but happy to take a look if there's some PR out there. This is not something that happens in the core component:
Gravacao.do.ecra.2024-05-02.as.14.22.32.mov
There was a problem hiding this comment.
In pages we have preloaded the data (like Templates) this doesn't occur. In page though it does and it seems related to these lines of code: https://github.com/WordPress/gutenberg/blob/trunk/packages/edit-site/src/components/page-pages/index.js#L65. Did you have similar code in your explorations @roo2 ?
We need to check this either way. -cc @jorgefilipecosta
There was a problem hiding this comment.
@ntsekouras I think the reason for the issue in our code is that we are updating the URL to include the filter, pagination params e.g. ?page=2 etc. This is causing the dataviews component to be re-rendered, which causes the line you mentioned to be hit several times and then the line I changed gets hit which would have reset the pagination before applying my change.
There was a problem hiding this comment.
I can try to create a little demo or test case with re-rendering to prove this
jameskoster
added
the
[Feature] DataViews
|
@ntsekouras, @oandregal the issue is that the component fires the onChangeView even when it first gets rendered. In calypso, we are re-rendering the component after opening a site because we save the selected site in the URL. So this causes the I've added a test which demonstrates this, previously the first test |
|
So another impact is if you tried to render a dataviews that started on page two for example (in our case, because we restored the state saved in the URL), the page would be reset on first render. |
|
@ntsekouras, @oandregal, would you mind taking another look please 🙏, I think it's quite a safe change now with tests etc and it's quite important for our use case in calypso. |
7 tasks
7 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
I came across this while trying to use the component in calypso, I haven't been able to isolate the issue and reproduce it in a storybook.
The component is used in calypso to show a list of sites, clicking on a site opens a panel and reconfigures the layout.
However this is also causing this
useEffectblock withinsearch.jsto be hit, this resets the pagination control back to page one.I confirmed that this line was causing the problem in calypso and confirmed that this would fix the issue and not cause regressions with search.
Before: (The page selector is reset when selecting a site)
Screen.Recording.2024-05-01.at.4.47.37.pm.mov
After:
Screen.Recording.2024-05-02.at.3.46.08.pm.mov
Testing instructions
Unfortunately, I've been unable to isolate it to the point where I can see exactly why this is happening, I think it would require a very deep understanding of that calypso code and the react rendering cycle. It is a lot easier to simply fix this issue in the DataView component and reason about the code without being able to make a test case for it.
I did a regression test by running
npm run storybook:devand going tohttp://localhost:50240/?path=/story/dataviews-dataviews--default