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(multi-tags): add filtering to collections #916

Merged
merged 22 commits into from
Dec 20, 2024
Merged

Conversation

seaerchin
Copy link
Contributor

@seaerchin seaerchin commented Dec 3, 2024

Problem

This PR adds filtering to collections through the use of tags.

Solution

  1. add Tags to the schema on CollectionPagePageSchema
    • note that in our publishing script, we explictly check for whether it's a PAGE_RESOURCE_TYPE, which means that even though the schema can only expose it on page items. we might want to expose it on files, which would necessitate a slight change inhow we generate hte SitemapEntries. not too sure if this is what we want, so i omitted it for now. need to think abit more here
  2. add tags to publishing/index.ts so that it gets written to the sitemap entry (and hence, to disk)
  3. in Collection, extract the tags from the Sitemap. add filtering logic onto our collection so that it will display the correct cards on filter click. take note that the logic here can be (and should be) reused for collection but not for years due to how the comparison is made
    • i didn't do it for collections because the id and the check is a lower-cased check via a hard-coded id + the property is different (takes from item.category and not item.tags

Videos

Screen.Recording.2024-12-03.at.10.40.57.PM.mov

@seaerchin seaerchin requested a review from a team as a code owner December 3, 2024 14:35
Copy link

vercel bot commented Dec 3, 2024

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

Name Status Preview Comments Updated (UTC)
isomer-studio ✅ Ready (Inspect) Visit Preview 💬 Add feedback Dec 9, 2024 3:46am

@datadog-opengovsg
Copy link

datadog-opengovsg bot commented Dec 3, 2024

Datadog Report

Branch report: feat/collection-filters
Commit report: 2917914
Test service: isomer-studio

✅ 0 Failed, 232 Passed, 36 Skipped, 38.29s Total Time
🔻 Test Sessions change in coverage: 1 decreased (-0.05%)

🔻 Code Coverage Decreases vs Default Branch (1)

  • vitest run --coverage 3.89% (-0.05%) - Details

@@ -3,6 +3,8 @@ import type { IsomerSiteProps, LinkComponentType } from "~/types"

export interface Tag {
label: string
category: string
values: string[]
Copy link
Contributor

Choose a reason for hiding this comment

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

actually for starters, we discussed that the entire universe of values allowed we will only know at build time till we achieve studio compatibility

This is because the tags are stored on an collection-item level

Is that still the case?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yea, this is taken at build time. you can see this at publishing/index.ts where we write the tag to the sitemap

Copy link
Contributor

Choose a reason for hiding this comment

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

sorry not sure if im understanding correctly, but why do we need both selected and values? seems to me that

  • selected is a subset of values
  • and since total possible tag values are computed on build time, then we don't really need values, but we can just do a "concat + uniq" of all selected ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm yea, similar thoughts. I didn't think we will need the values array here. In the long term, this will be moved to the collection level when it will be required.

However, for current implementation, values will not be known till build time

Copy link
Contributor

Choose a reason for hiding this comment

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

correct me if im wrong, but will not knowing the values till build time be an issue?

seems like we only require all values for the filters and options, which is fine to only be known during build time

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yea, you're right - i thought we were gonna ask the users for the set of values then display them in the filters (eg: a: 0) but actually we'll hide those wtihout any matches so i'll remove this

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@adriangohjw - not sure i understand your question! i think we don't require the values at all - i think, for now, we can just fetch all values from db (collections are 1 level deep), which shuold still be manageable

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated!

Copy link
Contributor Author

seaerchin commented Dec 20, 2024

Merge activity

  • Dec 19, 10:18 PM EST: A user started a stack merge that includes this pull request via Graphite.
  • Dec 19, 10:21 PM EST: Graphite rebased this pull request as part of a merge.
  • Dec 19, 10:29 PM EST: A user merged this pull request with Graphite.

@seaerchin seaerchin changed the base branch from feat/collection-tags to graphite-base/916 December 20, 2024 03:19
@seaerchin seaerchin changed the base branch from graphite-base/916 to main December 20, 2024 03:19
@seaerchin seaerchin force-pushed the feat/collection-filters branch from 972af8c to ffc95fa Compare December 20, 2024 03:20
@seaerchin seaerchin merged commit 5a0713b into main Dec 20, 2024
19 of 20 checks passed
@seaerchin seaerchin deleted the feat/collection-filters branch December 20, 2024 03:29
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.

3 participants