-
Notifications
You must be signed in to change notification settings - Fork 1
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
Enhancement - collection do not render empty filter #972
Enhancement - collection do not render empty filter #972
Conversation
Datadog ReportBranch report: ✅ 0 Failed, 257 Passed, 36 Skipped, 47.25s Total Time |
packages/components/src/templates/next/components/internal/BlogCard/BlogCard.tsx
Outdated
Show resolved
Hide resolved
packages/components/src/templates/next/layouts/Collection/filterUtils/index.ts
Outdated
Show resolved
Hide resolved
packages/components/src/templates/next/layouts/Collection/utils.ts
Outdated
Show resolved
Hide resolved
packages/components/src/templates/next/layouts/Collection/filterUtils/getTagFilters.ts
Outdated
Show resolved
Hide resolved
…s.ts Co-authored-by: Hsu Zhong Jun <27919917+dcshzj@users.noreply.github.com>
const items: ProcessedCollectionCardProps[] = [ | ||
{ | ||
category: "guides", | ||
} as ProcessedCollectionCardProps, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
issue: this type-cast is a little weird. I understand that we just want to test specific parts of the collection card props but this might hide problems if the implementation were to be changed, or if the props has changed upstream.
We can probably create a few complete test objects to be used across the different tests, or have some builder function that will automatically populate the remaining fields so that we can avoid this type casting.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
might hide problems if the implementation were to be changed, or if the props has changed upstream.
hmm not quite understanding this, as this test should fail if the implementation changes (e.g. using more props), which should be the intended outcome of a test?
getCategoryFilter(items), | ||
getYearFilter(items), | ||
...getTagFilters(items), | ||
].filter((filter) => filter.items.length >= 1) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: I think we might be able to remove this filter? There shouldn't be a case where a filter exists but there are 0 items.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
currently the get*Filter
functions returned a Filter
object regardless, and they could be empty items
for example, if all the lastUpdated
are undefined (it's an optional field, though it auto set to Date.now() in Studio UI on initialization)
thought that we can keep this as a more defensive approach
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approving first due to urgency, let's address the type-casting issue in a separate PR
Problem
We are returning filters with only one filter option. This doesn't make sense since clicking on it has no impact - it will only clutter the UI
Solution
Breaking Changes
Improvements:
Tests