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

feat: preserve list state between form navigation #425

Merged
merged 5 commits into from
Oct 22, 2024

Conversation

Birkbjo
Copy link
Member

@Birkbjo Birkbjo commented Oct 21, 2024

There's been feedback from testers that it can be distruptive to loose the list state when navigating between the form and list. This PR implements enhancements to preserve the list state (eg. the query-params used to store list-filters and pagination).

Implementation details

  • I really didn't want to keep a different store (eg. context or zustan) and keep that in sync with query-params. So I decided to try out location.state.
  • Query-params are stored as the verbatim serialized search-params string in location.state when navigating from a link that navigates to the form. This is done by passing state to navigate or as a prop to react-routers Link-component.
  • This way we keep all logic of useQueryParams the same - and we reapply it by appending the search-params to links that link back - by setting search: location.state.search. The same way is done if we use useNavigate or useLinkClickHandler.
  • One "drawback" with this approach is that we have to ensure that all links back and forth apply the state correctly. I've tried to "abstract" this in useLocationSearchState - but it's still up to the Link and navigation components to pass the states correctly.

Copy link

netlify bot commented Oct 21, 2024

Deploy Preview for dhis2-maintenance-app-beta ready!

Name Link
🔨 Latest commit 4ab1781
🔍 Latest deploy log https://app.netlify.com/sites/dhis2-maintenance-app-beta/deploys/6717b31720300f000965dff9
😎 Deploy Preview https://deploy-preview-425--dhis2-maintenance-app-beta.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@codecov-commenter
Copy link

Codecov Report

Attention: Patch coverage is 55.88235% with 15 lines in your changes missing coverage. Please review.

Project coverage is 60.78%. Comparing base (a8dfc9d) to head (6eef4f2).
Report is 14 commits behind head on master.

Files with missing lines Patch % Lines
src/lib/routeUtils/useLocationSearchState.ts 65.21% 8 Missing ⚠️
...nts/sectionList/listActions/SectionListActions.tsx 50.00% 3 Missing ⚠️
src/components/form/DefaultFormContents.tsx 0.00% 2 Missing ⚠️
src/lib/form/useOnSubmit.ts 0.00% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #425      +/-   ##
==========================================
- Coverage   62.05%   60.78%   -1.28%     
==========================================
  Files         192      207      +15     
  Lines        2991     3302     +311     
  Branches      669      733      +64     
==========================================
+ Hits         1856     2007     +151     
- Misses       1132     1287     +155     
- Partials        3        8       +5     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@tomzemp tomzemp self-requested a review October 21, 2024 13:26
@Birkbjo Birkbjo force-pushed the feat/persist-list-filter-state branch from 795c70b to 88077f2 Compare October 21, 2024 14:57
Copy link
Member

@tomzemp tomzemp left a comment

Choose a reason for hiding this comment

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

I think it looks good. Seems reasonable to use the search state and I like the effort to centralize some of the logic in useLocationSearchState.

One "drawback" with this approach is that we have to ensure that all links back and forth apply the state correctly. I've tried to "abstract" this in useLocationSearchState - but it's still up to the Link and navigation components to pass the states correctly.

Just to point out the pitfall here but the data element forms are not using the DefaultFormContents and hence do not have this updated navigation. Though, within the form, it seems pretty easy to apply this logic. If you wanted to abstract it more, I guess you could have defaultCancel or defaultSubmit actions that get extended? I guess it might depend on how much custom form logic we think we're going to implement.

I really didn't want to keep a different store (eg. context or zustan) and keep that in sync with query-params. So I decided to try out location.state

I think the approach is good. Though I guess one downside of not keeping it in a store is that you're going to lose the state if you say, navigate to Categories add some filters, navigate to Category Combos, navigate back to Categories. I assume though that the need to persist the form filtering is just while you're working on a particularly metadata object type?

@Birkbjo
Copy link
Member Author

Birkbjo commented Oct 22, 2024

Just to point out the pitfall here but the data element forms are not using the DefaultFormContents and hence do not have this updated navigation. Though, within the form, it seems pretty easy to apply this logic. If you wanted to abstract it more, I guess you could have defaultCancel or defaultSubmit actions that get extended? I guess it might depend on how much custom form logic we think we're going to implement.

Yeah, thanks! I'm aware and will push a fix. The intention is to refactor data elements at some point as well. But yeah, the other ones use useOnSubmit[Edit/New] which is intended to be the default. Right now they can be extended in the way of passing a transformer, but I could also extract some of the internal logic in the hooks to be easier to compose, as you say.

I really didn't want to keep a different store (eg. context or zustan) and keep that in sync with query-params. So I decided to try out location.state

I think the approach is good. Though I guess one downside of not keeping it in a store is that you're going to lose the state if you say, navigate to Categories add some filters, navigate to Category Combos, navigate back to Categories. I assume though that the need to persist the form filtering is just while you're working on a particularly metadata object type?

Yeah this is a good point. I have tried to gather specs, and discussed with Karoline about of the desired behaviour. And for now I think we should keep it local.
If we really wanted to keep it globally, we can add it to a store I guess. However, the main reason I didnt want to do it is that the "apply" logic could get messy and buggy. Eg. You dont want to just so it on first render due to loading logic, and you dont want to apply when empty. So I just didn't want to deal with that. But there's certainly ways to solve that.

@Birkbjo Birkbjo merged commit da1f11f into master Oct 22, 2024
10 checks passed
@Birkbjo Birkbjo deleted the feat/persist-list-filter-state branch October 22, 2024 14:38
dhis2-bot added a commit that referenced this pull request Oct 22, 2024
# [0.9.0](v0.8.0...v0.9.0) (2024-10-22)

### Features

* new org unit form ([cee3edc](cee3edc))
* preserve list state between form navigation ([#425](#425)) ([da1f11f](da1f11f))
@dhis2-bot
Copy link
Contributor

🎉 This PR is included in version 0.9.0 🎉

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
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants