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

fix: use-query-param replaceIn instead of pushIn #60

Open
wants to merge 1 commit into
base: development
Choose a base branch
from

Conversation

Birkbjo
Copy link
Member

@Birkbjo Birkbjo commented May 6, 2022

While making selections in the context-selection, the history stack is "spammed" with new entries when making initial selections. This changes it so new entries are only pushed when it's a valid selection - reducing entries in the history that are not really useful.

@Birkbjo Birkbjo requested a review from Mohammer5 May 6, 2022 13:39
@dhis2-bot
Copy link
Contributor

🚀 Deployed on https://pr-60--dhis2-data-entry.netlify.app

@Birkbjo
Copy link
Member Author

Birkbjo commented May 6, 2022

Not sure what to do about the test, since hook should not really be tested outside of a functional component... Trying to fix the test by fixing the signature of the mocked use-query-params just results in Invalid hook call. Hooks can only be called inside of the body of a function component.

Comment on lines +20 to +21
const [paramValue, setParamValue] = useQueryParam(name, schema)
const isValidSelection = useIsValidSelection()
Copy link
Collaborator

Choose a reason for hiding this comment

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

How does it know whether it's a valid selection before the change has been pushed to use-query-param.
So for example a data set has a default cat combo, when the data set id and the org unit id is already present and the user selects a period, when the logic reaches this point, the useIsValidSelection hook doesn't know about the period id yet, does it?

@Mohammer5
Copy link
Collaborator

Regarding the test:

  • You move useCustomQueryParam into a separate file
  • In the test, you replace every occurance of useQueryParam with useCustomQueryParam

That should get the test assing already, then I suggest to also add a separate test for use-custom-query-params.js

@HendrikThePendric
Copy link
Contributor

While making selections in the context-selection, the history stack is "spammed" with new entries when making initial selections. This changes it so new entries are only pushed when it's a valid selection - reducing entries in the history that are not really useful.

This seems like it would be a very nice improvement to add @Birkbjo. Are we deferring this due to time constraints?

@Mohammer5
Copy link
Collaborator

@Birkbjo could you just write a quick explanation on why this is neither continued nor merged? Couple of sentences should suffice

@Mohammer5
Copy link
Collaborator

By @Birkbjo:

also something we might investigate again, but pretty low priority, might not be something we need

@stale
Copy link

stale bot commented Apr 19, 2023

Hi!

Due to a lack of activity on this issue over time (180 days) it seems to be stale. If still relevant, please provide information that moves it forward, e.g. additional information, a pull request with suggested changes, or a reason to keep it open.

Any activity will keep it open, otherwise it will be closed automatically in 30 days. Thanks! 🤖

@stale stale bot added the stale label Apr 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants