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.

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

Add mixin #228

Merged
merged 2 commits into from
Jan 8, 2017
Merged

Add mixin #228

merged 2 commits into from
Jan 8, 2017

Conversation

thatkookooguy
Copy link
Member

Change Summary

Most functions in lodash's mixin are there as syntactic sugar. in chance.js, it just generates specific objects. this is nice for mocking small things while developing.

⚠️ MAKE SURE TO GO THROUGH THIS LIST ⚠️

(reviewers shouldn't approve a PR without the next three checkboxs checked)

Before you submit a PR, make sure you did the following things:

  • did you link this PR to an issue?

Make sure there's an issue open about the change you did (open one if there isn't). link this PR to that issue by writing resolves #{{issue_number}}. If this PR had several mission, link each issue in its parallel mission.

  • did you lint your changes to both javascript and scss?

hound can be pretty rough at keeping our code clean and readable! make sure you format your code by running either gulp format or gulp lint and cleaning out all the lint errors. If you won't, you'll have a long conversation with hound :-)

  • "I'm pretty sure I'll be able to read and understand this PR, even if I wasn't the author." - said the PR author

@thatkookooguy thatkookooguy temporarily deployed to kibibit-demo-pr-228 January 8, 2017 12:00 Inactive
@thatkookooguy thatkookooguy requested a review from ortichon January 8, 2017 12:00
@thatkookooguy thatkookooguy self-assigned this Jan 8, 2017
@ortichon
Copy link
Member

ortichon commented Jan 8, 2017

@thatkookooguy
why do we include lodash in our lib folder instead of bower-install it?

@thatkookooguy
Copy link
Member Author

thatkookooguy commented Jan 8, 2017 •
edited
Loading

@ortichon lodash ditched bower :-/
They don't support it anymore

https://github.com/lodash/lodash/wiki/Changelog#v400

The lines I added is basically in app.js

@ortichon
Copy link
Member

ortichon commented Jan 8, 2017 •
edited
Loading

@thatkookooguy so maybe we should consider install it via npm? or even install all client 3rd parties via npm?
(it seems more and more developers ditching bower lately 👎 )

@thatkookooguy
Copy link
Member Author

@ortichon it's already installed using npm. I just copied it to another folder

I don't think it's necessary atm. Basically, this mostly affects our development environment since in production, gulp just merges all the files together so you don't need a lib public folder.
We can open a bug to do this later since it will probably be nice to advance to another build system.

But it's not as if there's a bug that happens because of it. And we can bypass lodash specifically pretty fast if we ever need to (by adding 2 lines to our gulpfile). That's why I think dealing with this atm is not a top priority

@thatkookooguy
Copy link
Member Author

I can change it specifically for lodash if you want

@thatkookooguy
Copy link
Member Author

We don't use LGTM anymore :-)

just use the normal github review thingy :-)

ortichon
ortichon approved these changes Jan 8, 2017
View reviewed changes
Copy link
Member

@ortichon ortichon left a comment

Choose a reason for hiding this comment

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

LGTM,
You can change it only for lodash if you wish, I accept your explanations, so I don't care that it'll stay like this for now

@thatkookooguy
Copy link
Member Author

It will be in another pull request

go ahead and merge this one

thanks!

@ortichon ortichon merged commit be74983 into master Jan 8, 2017
@ortichon ortichon deleted the add-mixin branch January 8, 2017 12:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ortichon ortichon ortichon approved these changes

Assignees

@thatkookooguy thatkookooguy

Labels
None yet
Projects
None yet
Milestone
01. Create a kickass **code editor** ...
Development

Successfully merging this pull request may close these issues.

Replace underscore with lodash + add my custom mixins
2 participants
@thatkookooguy @ortichon