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

Added index files to import from and added test script so Grunt doesn't have to be installed globally #17

Merged
merged 9 commits into from
Oct 31, 2017
Merged

Conversation

Jahans3
Copy link
Contributor

@Jahans3 Jahans3 commented Oct 27, 2017

No description provided.

@esr360
Copy link
Collaborator

esr360 commented Oct 28, 2017

@Jahans3 pls fix js hint error

@Jahans3
Copy link
Contributor Author

Jahans3 commented Oct 30, 2017

@esr360 fixed errors and made a couple more changes

@Jahans3 Jahans3 closed this Oct 30, 2017
@Jahans3 Jahans3 reopened this Oct 30, 2017

exports.component = (query, operator, target = domNodes) => component({
target,
module,
Copy link
Collaborator

Choose a reason for hiding this comment

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

module, is the same as module: module, ?!?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, if you just pass the identifier you get that as the key and the value is the value of the variable

Copy link
Collaborator

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

* @param {Object} [parser] - custom parser to use for configuration
* @returns {*}
*/
export function getOptions ({ config = {}, parser, custom = {} } = {}) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why ({ config = {}, parser, custom = {} } = {}) { instead of (config = {}, parser, custom = {}) {?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Using named parameters is just a good habit to get into.

They allow you to pass arguments in at any position, so it's one less thing that can go wrong.

Partial application (not always using all parameters) is simpler since you don't need to ever do anything like: someFunc(arg1, null, arg3).

You can also easily rename them too: ({ arg: someArg }) => {}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I mean it's technically just destructuring an object but it's basically named params

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also means you can make the params non optional and throw an error by just leaving out the = {} at the very end

Copy link
Collaborator

@esr360 esr360 Oct 31, 2017

Choose a reason for hiding this comment

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

After like an hour reading I get it now! It just looks weird having things like = {} } = {}) { in your code but yeah it makes sense

@esr360
Copy link
Collaborator

esr360 commented Oct 31, 2017

Will pull it and run a smoke bomb local test to ensure it's all good then will grit push submerge the PR

@esr360 esr360 merged commit 95efa65 into One-Nexus:master Oct 31, 2017
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