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

Fix Code Style and Bugs in Module Examples #15572

Merged
merged 2 commits into from
Jan 16, 2019
Merged

Conversation

gkjohnson
Copy link
Collaborator

@gkjohnson gkjohnson commented Jan 13, 2019

  • Adds missing semicolons and some other code style issues
  • Fixes lingering references to THREE. in MapControls, OrbitControls, and TrackballControls, making them unusable.

@donmccurdy
Copy link
Collaborator

I'm not sure at what point we're ready to call the ES modules the canonical versions (@mrdoob any preference? case by case or all at once?). There are PRs open against GLTFLoader, and we'll presumably want to re-run the script if we merge them as-is. IMO, once we start making hand edits to the ES modules, we should begin autogenerating the UMD files and putting the header on them to direct future PRs to the right place.

Any issues that could be fixed in modularize.js we should fix there.

@gkjohnson
Copy link
Collaborator Author

Updated the script in #15582

@gkjohnson
Copy link
Collaborator Author

Would it make sense to move only some of the example modules over be the canonical versions? Those that aren't in active development or don't have open PRs, for example. It may be best to make the conversion when it seems convenient for each script otherwise it'll be hard to find the moment to turn it on for every completed module. MapControls and TransformControls don't have anything open for them, for example.

I'm thinking something like an ignore list in the convert-examples rollup config?

@donmccurdy
Copy link
Collaborator

Would it make sense to move only some of the example modules over be the canonical versions?

I'm OK with that, but would defer to @mrdoob... having an ignore list and appropriate headers on the files that aren't meant to be edited (whether that's in js/ or jsm/) would also help manage the transition.

If there aren't that many open PRs against examples and we'd rather do this all at once, I'm fine with that too.

@mrdoob mrdoob added this to the r101 milestone Jan 16, 2019
@mrdoob mrdoob merged commit 017f2a9 into mrdoob:dev Jan 16, 2019
@mrdoob
Copy link
Owner

mrdoob commented Jan 16, 2019

Thanks!

@mrdoob
Copy link
Owner

mrdoob commented Jan 16, 2019

I'm not sure at what point we're ready to call the ES modules the canonical versions (@mrdoob any preference? case by case or all at once?).

I would go case by case I think. Seems like we can start with controls.

@gkjohnson gkjohnson deleted the jsm-linting branch January 16, 2019 02:09
@gkjohnson
Copy link
Collaborator Author

I would go case by case I think. Seems like we can start with controls.

Great -- when I get a chance I'll add an ignore list for the module conversions and add appropriate banners to the non-canonical versions of the example scripts.

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.

3 participants