-
-
Notifications
You must be signed in to change notification settings - Fork 90
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
Provide beta fox assets and demo #50
Conversation
5a458dd
to
fa866a2
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
I'd probably rather we pass the mesh json into the constructor than have a beta flag, because this implies we package all meshes with every distribution, when instead we could be building towards a modularity where we can swap out logos. |
9a0daf5
to
bd38c22
Compare
Note that this is a breaking change as it no longer defaults to showing the standard fox logo -- it must now be provided via |
index.js
Outdated
@@ -11,6 +10,10 @@ const { | |||
module.exports = createLogo | |||
|
|||
function createLogo (options = {}) { | |||
if (!options.meshJson) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have you considered making fox.json
the default meshJson
rather than making it a strict requirement? That would let us release this as a non-breaking change (I think).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could, yes; I got the impression from @danfinlay's comment that we should keep this package as small as possible, but yeah, I'll go back to a default.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good! Using fox.json
by default shouldn't affect package size - it was still being included. Unless you meant to remove it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
Provides a
beta
option to define if we want that fox. Opendocs/beta/index.html
to check it out.