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

Add findByTag and replace underscore with lodash #3

Merged
merged 2 commits into from
May 23, 2014

Conversation

jamlen
Copy link
Contributor

@jamlen jamlen commented May 23, 2014

Allows finding articles by tag. I had to replace underscore with lodash, but see this issue which has a long discussion about it and its performance, and the conclusion was to switch to lodash.

Url this uses is /tag/{{tag}}

@jamlen
Copy link
Contributor Author

jamlen commented May 23, 2014

Next I'm looking at pagination... 😉

if (tag) {
var results = _.where(blogs, { tags: [tag] });

if (results !== null && results !== undefined) {
Copy link
Owner

Choose a reason for hiding this comment

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

Does lo-dash's where ever return null or undefined? Seems to always return an array, either with items if something is matched, or empty otherwise. Given that no tag argument would mean tag === undefined, could we simplify the entire function to:

var findByTag = function(tag) {
  _reinit();
  return _.where(blogs, { tags: [tag] });
};

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're right! https://github.com/lodash/lodash/blob/2.4.1/dist/lodash.compat.js#L3483 looks like where is an alias for filter which always returns the array. Will update and commit...

Copy link
Owner

Choose a reason for hiding this comment

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

Awesome 👍

@martinrue
Copy link
Owner

Nice, this is a good idea :)

I've left one comment regarding findByTag implementation. Let me know what you think and let's get this merged.

Thanks!

@jamlen
Copy link
Contributor Author

jamlen commented May 23, 2014

Done! merge away.... and I'll change that in my sitemark repo too!

@martinrue
Copy link
Owner

Thanks again James :)

martinrue added a commit that referenced this pull request May 23, 2014
Add findByTag and replace underscore with lodash
@martinrue martinrue merged commit c5beb68 into martinrue:master May 23, 2014
@jamlen jamlen deleted the find-by-tag branch May 23, 2014 21:56
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.

2 participants