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

Fixed malfunction of patient filter #5328

Closed
wants to merge 11 commits into from

Conversation

patelaryan7751
Copy link
Contributor

@patelaryan7751 patelaryan7751 commented Apr 12, 2023

WHAT

🤖 Generated by Copilot at f537466

This change improves the filter functionality of the patient component by using a useEffect hook to sync the local filter state with the filter prop from the parent component. It affects the file src/Components/Patient/PatientFilter.tsx.

Proposed Changes

filter.mp4

@coronasafe/care-fe-code-reviewers @coronasafe/code-reviewers

Merge Checklist

  • Add specs that demonstrate bug / test a new feature.
  • Update product documentation.
  • Ensure that UI text is kept in I18n files.
  • Prep screenshot or demo video for changelog entry, and attach it to issue.
  • Request for Peer Reviews
  • Completion of QA

HOW

🤖 Generated by Copilot at f537466

  • Add a useEffect hook to synchronize the filter state with the parent component (link)

@vercel
Copy link

vercel bot commented Apr 12, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
care-storybook ✅ Ready (Inspect) Visit Preview 💬 Add feedback Apr 19, 2023 4:11pm

@netlify
Copy link

netlify bot commented Apr 12, 2023

Deploy Preview for care-egov-staging ready!

Name Link
🔨 Latest commit 4b942dd
🔍 Latest deploy log https://app.netlify.com/sites/care-egov-staging/deploys/6440127718c885000735e61d
😎 Deploy Preview https://deploy-preview-5328--care-egov-staging.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 settings.

@nihal467
Copy link
Member

nihal467 commented Apr 13, 2023

@patelaryan7751
image

the facility name and location is not getting removed in the asset tab, when we close the badge

Copy link
Member

@rithviknishad rithviknishad left a comment

Choose a reason for hiding this comment

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

@patelaryan7751 it seems like we are introducing quite a few hacks to manage something simple.

It did work before without these many hacks before. Isn't it as simple as getting the filter state from query parama, resolving it to object by the parent component, passing the resolved filter objects to the FiltersSlideOver component without needing to manage too many states inside the FiltersSlideOver component itself as it's already managed by the parent?

cc: @khavinshankar

Copy link
Member

Choose a reason for hiding this comment

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

@patelaryan7751 could you add doc comments to explain what this does? or maybe give more appropriate name for the function?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@rithviknishad Yeah sure actually this function checks whether 2 JSON objects passed are equal or not irrespective of the order in which they are provided.

Comment on lines +64 to +68
useEffect(() => {
if (!isPreventChangeFilterStateOn) {
setFilterState(filter);
}
}, [filter]);
Copy link
Member

Choose a reason for hiding this comment

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

not a fan of this hack.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@rithviknishad This actually prevents the sync of query params and the filterState of advanced filter.

Copy link
Member

Choose a reason for hiding this comment

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

Why would you need that to be prevented?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@rithviknishad There are many state changes happening to the filterState . So I want the changes to be synced only when someone clicks on apply filter button.

@patelaryan7751
Copy link
Contributor Author

@patelaryan7751 it seems like we are introducing quite a few hacks to manage something simple.

It did work before without these many hacks before. Isn't it as simple as getting the filter state from query parama, resolving it to object by the parent component, passing the resolved filter objects to the FiltersSlideOver component without needing to manage too many states inside the FiltersSlideOver component itself as it's already managed by the parent?

cc: @khavinshankar

@rithviknishad Yes it is simple to just pass the query param to the filterState of respective advanced filters but there are fields that cannot be directly assigned from query params specifically the fields which are involving selecting facility from the dropdown. There is a different state variable prefix()_ref type in the filterState which contains all info of the selected facility we need to assign it to null in order to clear the advanced filter state when a badge is cleared. So, directly passing it to the parent state need some modifications, which I did, using useEffect hook . In simple terms, I am trying to sync the filterState of advanced filters with the query params when there is any change caused by badges outside. If there is any other method to do this then I can rework on it.

@sonarcloud
Copy link

sonarcloud bot commented Apr 19, 2023

SonarCloud Quality Gate failed.    Quality Gate failed

Bug C 2 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 1 Code Smell

No Coverage information No Coverage information
4.4% 4.4% Duplication

@rithviknishad
Copy link
Member

rithviknishad commented Apr 19, 2023

@patelaryan7751

I'm not sure if you understood it, let me rephrase.

The current filter state is stored in the URL's query params right? Which is a Record<key, value> where value is a string or a ref. ID as string, right?

The parent component (eg. AssetsList) is getting the query params useFilters and all ref. ID-based filters are being resolved to appropriate objects by the parent component itself right?

So before even you reach the AssetsFilter (Slideover) component, you already have a Record<key: FilterName, value: string | ResolvedObject> right?

Passing this to the slide-over should simply work right?

Now for the clearFilter, the behaviour is pretty simple and straightforward across all filters. It just needs to navigate to the URL without query params. Updating the state need not be done since the qParams automatically will be updated since URL has changed, hence relevant parts will be re-rendered and states get updated. So make it a generalized function coming from the filters slideover component itself, that does it since it's not dependent on which filters it is part of.

(I'm not sure if I'm missing something you are trying to convey, correct me if I'm wrong)

@github-actions github-actions bot added the merge conflict pull requests with merge conflict label Apr 21, 2023
@github-actions
Copy link

👋 Hi, @patelaryan7751,
Conflicts have been detected against the base branch. Please rebase your branch against the base branch.


This message is automatically generated by prince-chrismc/label-merge-conflicts-action so don't hesitate to report issues/improvements there.

@rithviknishad
Copy link
Member

Closing as you've continued in #5362

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.

New Slideover is malfunctioning across platform
3 participants