这是indexloc提供的服务，不要输入任何密码
Skip to content

fix: FEAT: Array column should be filterable #1385

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

fix: FEAT: Array column should be filterable #1385

wants to merge 1 commit into from

Conversation

@rishi-raj-jain
Copy link

@rishi-raj-jain rishi-raj-jain commented Aug 17, 2023 •
edited
Loading

fix: #1382
/claim #1382

@vercel
Copy link

vercel bot commented Aug 17, 2023

@rishi-raj-jain is attempting to deploy a commit to the Rowy Team on Vercel.

A member of the Team first needs to authorize it.

@vercel
Copy link

vercel bot commented Aug 17, 2023 •
edited
Loading

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

Name Status Preview Comments Updated (UTC)
rowy-os ❌ Failed (Inspect) Aug 19, 2023 1:06pm
1 Ignored Deployment
Name Status Preview Comments Updated (UTC)
rowy-typedoc ⬜️ Ignored (Inspect) Visit Preview Aug 19, 2023 1:06pm

@algora-pbc algora-pbc bot mentioned this pull request Aug 17, 2023
Closed
@rishi-raj-jain rishi-raj-jain changed the title create filters for Array fix: FEAT: Array column should be filterable Aug 17, 2023
@rishi-raj-jain
Copy link
Author

rishi-raj-jain commented Aug 18, 2023

@harinij @shamsmosowi

Can you take a look? I've extended every method in firestore for arrays: https://firebase.google.com/docs/firestore/query-data/queries#array_membership

Works locally just as fine.

@shamsmosowi shamsmosowi requested a review from il3ven August 19, 2023 13:02
@il3ven il3ven changed the base branch from main to develop August 20, 2023 14:24
@il3ven
Copy link
Member

il3ven commented Aug 20, 2023

@rishi-raj-jain First of all, thank you for the efforts. Second, the PR needs to be opened against develop. You'll need to rebase. Third, this PR isn't working for all cases. To reproduce, use "not-includes" as the operator.

I believe it isn't working because the translation from value to firebase query is missing. It should be done here.

rowy/src/hooks/useFirestoreCollectionWithAtom.ts

Lines 386 to 398 in b2983f4

} else if (filter.operator === "time-minute-equal") {
if (!filter.value) continue;
const filterDate =
"toDate" in filter.value ? filter.value.toDate() : filter.value;
const [startDate, endDate] = getTimeRange(filterDate);
firestoreFilters.push(where(filter.key, ">=", startDate));
firestoreFilters.push(where(filter.key, "<=", endDate));
continue;
} else if (filter.operator === "id-equal") {
firestoreFilters.push(where(documentId(), "==", filter.value));
continue;
} else if (filter.operator === "color-equal") {

@il3ven il3ven mentioned this pull request Aug 20, 2023
Closed
@rishi-raj-jain
Copy link
Author

rishi-raj-jain commented Aug 20, 2023

Thanks for pointing that out.
So I see this only failing for both includes and non-includes. The rest seem to work fine.
By against develop you mean from my develop to rowy's develop, right? Right now, it's from my main to your's develop.

@il3ven
Copy link
Member

il3ven commented Aug 20, 2023

So I see this only failing for both includes and non-includes. The rest seem to work fine.

Yes

By against develop you mean from my develop to rowy's develop, right? Right now, it's from my main to your's develop.

The name of your branch can be anything. It doesn't have to be develop. Let's say your branch name is rishi. The PR will be from rishi to develop. However, rishi should have all the commits from rowy's develop plus your new commits. Otherwise, there will be merge conflicts which we can see right now.

There are different ways to do this.

  • You can create a new branch from develop and cherry-pick the new commits from rishi-raj-jain:main.
  • Don't create a new branch but do git rebase develop to your current branch.

@rishi-raj-jain
Copy link
Author

rishi-raj-jain commented Aug 20, 2023

@il3ven

Thanks for the answers. I do realize it's not the same but the branch.

I figured out that in and not-in don't work when we want to filter when the filter value is an array itself.

Only, ==, !=, array-contains and array-contains-any are the supported methods as I gather from that link + testing in a simple demo.js file.

@rishi-raj-jain rishi-raj-jain closed this by deleting the head repository Aug 20, 2023
@rishi-raj-jain
Copy link
Author

rishi-raj-jain commented Aug 20, 2023

Creating a fresh PR.

@rishi-raj-jain rishi-raj-jain mentioned this pull request Aug 20, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@il3ven il3ven Awaiting requested review from il3ven

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

FEAT: Array column should be filterable

2 participants

@rishi-raj-jain @il3ven