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

Adding some basic search capabilities #69

Closed
wants to merge 4 commits into from
Closed

Conversation

bmartus
Copy link
Contributor

@bmartus bmartus commented Feb 3, 2016

I needed a way to see if a certain type of product was in the users cart. This allows you to send an array of key-val pairs and it will return an array of items where all options match.

Let me know if you can think of anything else to add or test, I don't mind updating.

@lukepolo
Copy link
Owner

lukepolo commented Feb 3, 2016

Awesome only suggestion is to return the items that match as well ? Thoughts ?

@bmartus
Copy link
Contributor Author

bmartus commented Feb 3, 2016

The returned array contains all matching cart items. Or did you mean inspect those in the tests, rather than just count the size of the array?

@lukepolo
Copy link
Owner

lukepolo commented Feb 3, 2016

Yah for the test just make sure the item when returned actually has those options

@bmartus
Copy link
Contributor Author

bmartus commented Feb 3, 2016

Will do.

{
foreach ($data as $key => $value) {
if ($this->$key !== $value) {
// all search criteria must match
Copy link
Owner

Choose a reason for hiding this comment

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

remove comment , no need

@bmartus
Copy link
Contributor Author

bmartus commented Feb 4, 2016

Let me know if you think any other tests would help.

@lukepolo
Copy link
Owner

lukepolo commented Feb 4, 2016

I just made a huge update to laracart. Sadly this caused conflicts with yours.

@lukepolo
Copy link
Owner

lukepolo commented Feb 4, 2016

ill look at it in a bit to merge.

@bmartus
Copy link
Contributor Author

bmartus commented Feb 5, 2016

I can move these changes into a new PR off master tonight if that's easier, too.

@lukepolo
Copy link
Owner

lukepolo commented Feb 5, 2016

That would be great.

On Fri, Feb 5, 2016 at 6:14 PM, Brandon Martus notifications@github.com
wrote:

I can move these changes into a new PR off master tonight if that's
easier, too.


Reply to this email directly or view it on GitHub
#69 (comment).

@bmartus
Copy link
Contributor Author

bmartus commented Feb 6, 2016

got it.

@bmartus bmartus closed this Feb 6, 2016
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.

None yet

2 participants