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: faq page filter #218

Merged
merged 7 commits into from
Jul 20, 2021
Merged

fix: faq page filter #218

merged 7 commits into from
Jul 20, 2021

Conversation

mazipan
Copy link
Member

@mazipan mazipan commented Jul 20, 2021

Closes #203

Description

Fix bug on FAQ filter that showing a wrong data when we search any keywords

Changes

  • Add new prop checkDocSize: boolean on SearchForm
  • Set to false on faq page.

Screenshots

Before

Screen Shot 2021-07-20 at 17 49 36

After

Screen Shot 2021-07-20 at 17 49 49

@netlify
Copy link

netlify bot commented Jul 20, 2021

✔️ Deploy Preview for wargabantuwarga ready!

🔨 Explore the source changes: 738c110

🔍 Inspect the deploy log: https://app.netlify.com/sites/wargabantuwarga/deploys/60f6be59546a890007bd338c

😎 Browse the preview: https://deploy-preview-218--wargabantuwarga.netlify.app

@mazipan
Copy link
Member Author

mazipan commented Jul 20, 2021

Help @ekamuktia to check

Copy link
Member

@zainfathoni zainfathoni left a comment

Choose a reason for hiding this comment

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

LGTM 💯

Thanks, Mas @mazipan for working on it.
Mbak @ekamuktia, please help to review these changes as well, thanks! 🙏

@zainfathoni zainfathoni enabled auto-merge July 20, 2021 12:15
@zainfathoni zainfathoni merged commit f3925bf into kawalcovid19:main Jul 20, 2021
@mazipan mazipan deleted the mazipan/fix-search-on-faq branch July 20, 2021 12:26
Copy link
Collaborator

@ekamuktia ekamuktia left a comment

Choose a reason for hiding this comment

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

Could you check on this part?
Sorry, it's a closed pull request but I just have time to check it.

Comment on lines +167 to +188
if (bucket.key) {
if (checkDocSize && bucket.doc_count > 0) {
return (
<option
key={`option-${key}-${bIdx + 1}`}
value={bucket.key}
>
{bucket.key}
</option>
);
}

// FAQ page doesn't check the doc_count
return (
<option
key={`option-${key}-${bIdx + 1}`}
value={bucket.key}
>
{bucket.key}
</option>
)
);
);
}
Copy link
Collaborator

@ekamuktia ekamuktia Jul 20, 2021

Choose a reason for hiding this comment

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

For this part, buckets that have doc_count 0 is still printed even though the checkDocSize is true because it goes to the 2nd return. So now, all the filters are always shown.
image

If the intention for FAQ page is to always show all the categories, another way to do it is to generate filterItems value from groupped faq instead of using value generated from useSearch.
This way, the filter category order can be in the same order as how the FAQ items are shown which probably is more natural for FAQ page.
image

Copy link
Member

Choose a reason for hiding this comment

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

Could you please report another issue for this Mbak @ekamuktia? So we can do a rework later. Thanks! 🙏

Copy link
Collaborator

Choose a reason for hiding this comment

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

Okay~!

Copy link
Member Author

@mazipan mazipan Jul 20, 2021

Choose a reason for hiding this comment

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

Check this PR #229

Copy link
Member Author

@mazipan mazipan Jul 20, 2021

Choose a reason for hiding this comment

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

For option no 2, I think it will need more time to be done, since it need to refactor the way we select the selected filter. It can be done on #197 if we want to pick the ticket

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

Successfully merging this pull request may close these issues.

Search Issue in FAQ page
4 participants