Closed jaimedp closed 6 years ago
Great! Adding ?w=1
to github diff url ignores whitespace so wasn't too bad to see changes :) Few thoughts:
nwt
publicly? Some examples like (http://forio.com/contour/gallery.html#/chart/scatter/scatter-trendline) depend on a few functions from that so we should.Contour.utils
instead?lodash
functions in contour-utils with the actual lodash imports? We can import those specific functions without bundling all of lodash.Also, in the spirit on modernizing jshintrc -> eslintrc ?
What're the before/after file-sizes of contour.min.js?
Contour.utils
to expose nwt to the rest of the world.Also, for publishing to npm, you'll need to create an npm user for forio and make sure the author
info in package.json
matches with the user, so we can publish it.
Looks good!
Lodash is overkill for the isObject
etc of course, but why not use it for the merge
, defaults
etc? The less code under our maintenance purview and the more we handoff to established libraries the better, unless there's a reason not to?
We should whitelist IE11 in preset-env.
I'll update the PR tonight.
One more thing to note. I removed the /dist folder from the repo, this means that you will no longer be able to use bower install
since it does not have a post-install build. Just to make sure you're ok completely dropping support for bower
.
Also, we need to remember to update contour.geo
too with this changes.
I'd forgotten bower existed till I tried to build contour last week, so fine dropping it :)
Tangentially, we've given up on making millions with Contour-advanced so contents of that repo should go somewhere public, just not sure where. Thoughts? Just new showcase examples? (I'll get you access to the advanced repo in a bit).
Moved to use lodash.merge
and lodash.defaults
(increase in size of the minified file by ~13k). Also added ie11 to the env in babel and fixed 267 & 268.
Regarding advance viz, I think it would be good if they had their place in te showcase and then you could just install them like npm install contour.drawable
or something like that
I'm having second thoughts about not including dist
, though more about workflow than anything technical.
For new PRs it's useful to have devs include a jsfiddle in their example pointing to a jsfiddle demoing their change, for e.g. see https://github.com/forio/contour/pull/251 which has http://jsfiddle.net/3yqyvdud/4/ which points to a pre-built contour file for that PR, pulled from the incoming dist. This really reduces the friction of testing PRs, and puts the onus of making sure it builds/works on the submitter.
Thoughts/ suggestions for a better workflow?
Hey, I see the point of evaluating PRs from a jsfiddle, but I think you'd end up with unnecessary merge conflicts which is minor, but the bigger problem I think it is that you end up with a very confusing versioning, since the /dist
folder no longer contains the "official" released version, it has whatever the latest merge puts there, if it didn't forget to build it... for example in cases where the PR does not need to be js tested (ie. changes to less files or docs), then the /dist
folder is not even the latest/edge version, is just a random snapshot.
There is one posible solution, but it would mean to drop the bundle of the lodash merge/default functions. The solution is to include the /src/scripts/index.js
file as a <script type="module">
that way (with some tweaks) it works, but it limits to import
only files addressable from the src folder (ie. no node_modules).
I did a proof of concept jsfiddle here
The changes required for this to work an on this commit
I still don't think it would be a good solution, since it imposes an unnatural limit to what you can use, but I wanted to test what it would look like :)
You can prevent merge conflicts / large diffs on dist if you mark minified files as "binary" in gitattributes (see https://github.com/forio/flow.js/blob/master/.gitattributes for e.g. https://blog.andrewray.me/dealing-with-compiled-files-in-git/ for more info).
I don't think dist contour.js being of uncertain freshness is that big a deal, if you're explicitly copying source from github instead of cdn/npm you hopefully know what you're doing :)
Source type module is an interesting idea, let me play around with that.
A lot of changes. I tried to do the messy change of removing and re-indenting files on the first commit and then fixes on the following commits to make it a bit easier to see, but it is still hard to see the changes :(