Skip to content
This repository has been archived by the owner on Jul 19, 2021. It is now read-only.

Add defaultTo function #114

Merged
merged 2 commits into from
Apr 7, 2017
Merged

Add defaultTo function #114

merged 2 commits into from
Apr 7, 2017

Conversation

jonathanmoore
Copy link
Contributor

What are you trying to accomplish with this PR?

Fixes #113
Following the lead of PR #78, Lodash's _.defaultTo function was added to slate.utils

Now the formatWithDelimiters function inside slate.Currency can correctly apply a default value of 2 if the precision argument equals 0—as well as any other place where a default variable needs to be set.

https://github.com/lodash/lodash/blob/master/defaultTo.js

slate.utils.defaultTo(1, 10)
// => 1

slate.utils.defaultTo(0, 10)
// => 0

slate.utils.defaultTo(undefined, 10)
// => 10

Checklist

For contributors:

For maintainers:

  • I have 🎩'd these changes.
  • I have bumped the package.json version in a separate PR, if applicable.

Copy link
Contributor

@NathanPJF NathanPJF left a comment

Choose a reason for hiding this comment

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

Some quick changes to addresss

* @param {*} defaultValue - Default value
* @returns {*} - Returns the resolved value
*/
function defaultTo(value, defaultValue) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This will need to be defined as a property of utils.

defaultTo: function(value, defaultValue) {
  return (value == null || value !== value) ? defaultValue : value
}

@@ -63,4 +63,19 @@ slate.utils = {
}
return result;
}

Copy link
Contributor

Choose a reason for hiding this comment

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

need a comma after the } above to add another property to slate.utils

@jonathanmoore
Copy link
Contributor Author

Thanks for catching that! This should take care of it.

@NathanPJF
Copy link
Contributor

Awesome! Thanks for both reporting the issue and getting the PR in @jonathanmoore

@NathanPJF NathanPJF merged commit 7c988ad into Shopify:master Apr 7, 2017
@jonathanmoore
Copy link
Contributor Author

🙌 I'm glad to help! I've been using Slate for the last few months when I converted our premium theme District over to support sections. It has been tremendously helpful.

@lock
Copy link

lock bot commented Oct 26, 2018

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@lock lock bot locked as resolved and limited conversation to collaborators Oct 26, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants