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

Cache only the advance filters #6988

Merged
merged 7 commits into from
Jan 17, 2024
Merged

Conversation

AshrafMd-1
Copy link
Contributor

Proposed Changes

@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

@AshrafMd-1 AshrafMd-1 requested a review from a team as a code owner January 6, 2024 15:44
Copy link

vercel bot commented Jan 6, 2024

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 Jan 16, 2024 2:47pm

Copy link

netlify bot commented Jan 6, 2024

Deploy Preview for care-egov-staging ready!

Name Link
🔨 Latest commit 39881b3
🔍 Latest deploy log https://app.netlify.com/sites/care-egov-staging/deploys/65a696c6d4c25000087e4e69
😎 Deploy Preview https://deploy-preview-6988--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 configuration.

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.

just some minor issues

src/Components/Patient/ManagePatients.tsx Outdated Show resolved Hide resolved
src/Common/hooks/useFilters.tsx Outdated Show resolved Hide resolved
src/Common/hooks/useFilters.tsx Outdated Show resolved Hide resolved
src/Common/hooks/useFilters.tsx Outdated Show resolved Hide resolved
src/Common/hooks/useFilters.tsx Outdated Show resolved Hide resolved
src/Common/hooks/useFilters.tsx Outdated Show resolved Hide resolved
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.

Everything LGTM.

Except skip keeping cacheBlacklist as an arg of updateFiltersCache.
Instead:

  • Skip passing cacheBlacklist as an arg to updateFitlersCache, there's no need for it to sit there.
  • Concat the cache blacklist outside the updateFiltersCache, because you need not keep concating everytime the function is invoked, you can just execute it while the useFilters hook runs.

@AshrafMd-1
Copy link
Contributor Author

@rithviknishad is this okay or are there any changes

@github-actions github-actions bot added the merge conflict pull requests with merge conflict label Jan 11, 2024
Copy link

👋 Hi, @AshrafMd-1,
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 rithviknishad added needs testing and removed changes required merge conflict pull requests with merge conflict labels Jan 11, 2024
@rithviknishad
Copy link
Member

@AshrafMd-1 was the filters cache working in your PR? How were you able to test whether this PR was working or not?

@rithviknishad rithviknishad added waiting for related PR a co-related detail PR is under construction and removed needs testing labels Jan 11, 2024
@AshrafMd-1
Copy link
Contributor Author

It wasn't working. Even when I tested it
I was looking at local storage to find if the variable is stored or not

@rithviknishad
Copy link
Member

No worries, #7009 would fix it. Once it's merged, you could rebase so that @nihal467 could test this out

@github-actions github-actions bot added the merge conflict pull requests with merge conflict label Jan 12, 2024
Copy link

👋 Hi, @AshrafMd-1,
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 rithviknishad added needs testing and removed waiting for related PR a co-related detail PR is under construction labels Jan 15, 2024
@nihal467 nihal467 added the cypress failed pull request with cypress test failure label Jan 16, 2024
@nihal467
Copy link
Member

@AshrafMd-1 fix the cypress test

@rithviknishad
Copy link
Member

@AshrafMd-1 the lint checks are failing

@AshrafMd-1
Copy link
Contributor Author

@rithviknishad @nihal467 done

@rithviknishad rithviknishad added needs testing reviewed reviewed by a core member and removed needs review changes required cypress failed pull request with cypress test failure labels Jan 17, 2024
@nihal467
Copy link
Member

LGTM

@khavinshankar khavinshankar merged commit afe04b9 into ohcnetwork:develop Jan 17, 2024
35 of 36 checks passed
Copy link

@AshrafMd-1 We truly appreciate your efforts. Thank you for taking the time to contribute; this is a very valuable contribution to us 🥇. We always welcome your contribution 🙂, so feel free to contribute to anything anytime, and never lose that spirit of innovation 🙌.

@AshrafMd-1 AshrafMd-1 deleted the Fix-#6891 branch January 24, 2024 08:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
reviewed reviewed by a core member tested
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cache only the advance filters across the platform
5 participants