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

Enhance useDataViewPagination with URL persisting #16

Merged
merged 17 commits into from
Aug 27, 2024

Conversation

fhlavac
Copy link
Collaborator

@fhlavac fhlavac commented Aug 26, 2024

RHCLOUD-34819

Enhanced useDataViewPagination hook with URL persisting and rebased to the latest main

chrome-capture-2024-8-26

@patternfly-build
Copy link

patternfly-build commented Aug 26, 2024

@@ -10,5 +14,5 @@ module.exports = {
'@semantic-release/npm'
],
tagFormat: 'prerelease-v${version}',
dryRun: false
dryRun: true

Choose a reason for hiding this comment

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

Is this intentional?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It's not, thank you

result.current.onSetPage(undefined, 4);
});

expect(mockSetSearchParams).toHaveBeenCalledTimes(2);

Choose a reason for hiding this comment

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

You check if the setter was called but not what values were passed to the argument.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Oh yeah I forgot to finish this one

updated && setSearchParams(params);
}
// eslint-disable-next-line react-hooks/exhaustive-deps
}, []);

Choose a reason for hiding this comment

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

I assume we are deliberately ignoring the changes in the inputs? But what if the URL changes by other means that just through the hook? Should the component react? Maybe the query values themselves should be in the dependency array.

setSearchParams,
}: UseDataViewPaginationProps) => {
const [ state, setState ] = useState({
page: searchParams?.get('page') !== null

Choose a reason for hiding this comment

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

I think you want some enum or constants for the query param names. Just so you don't have string around the code which might get tricky to update at some point.

@fhlavac fhlavac merged commit fc0d1fb into patternfly:main Aug 27, 2024
7 checks passed
Copy link

🎉 This PR is included in version 7.0.0-prerelease.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants