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

Some code cleanup #639

Merged
merged 22 commits into from
Nov 30, 2022
Merged

Some code cleanup #639

merged 22 commits into from
Nov 30, 2022

Conversation

vincentfretin
Copy link
Contributor

@vincentfretin vincentfretin commented Oct 6, 2022

  • Remove dead code, unused imports, unused variables.
  • Use import/export everywhere, latest wepback/babel don't allow to use module.exports if you used one import statement.
  • Specify a prettier config with option singleQuote to match the current code. I didn't reformatted the files with prettier in this PR.
  • Disable the eslint space-before-function-paren rule that conflicts with how prettier format the files.
  • Fix eslint errors.

@vincentfretin
Copy link
Contributor Author

eslint errors down to 15 errors instead of 350 errors

@vincentfretin
Copy link
Contributor Author

I'll fix the remaining eslint errors later then this PR should be good to merge if there is no objection.

@vincentfretin
Copy link
Contributor Author

I fixed all remaining eslint issues, excepted those:
npm run lint

src/components/components/Mixins.js
  20:5  error  Do not use setState in componentDidUpdate  react/no-did-update-set-state

I'll fix this one as part of the react update in #641.

src/index.js
  104:11  error  'geometry' is defined but never used  no-unused-vars
  105:11  error  'material' is defined but never used  no-unused-vars

This is already fixed in #638

This PR is ready to be merged after #638.

@vincentfretin
Copy link
Contributor Author

npm run prettier will reformat those files

	modifié :         src/components/Main.js
	modifié :         src/components/components/CommonComponents.js
	modifié :         src/components/components/Component.js
	modifié :         src/components/scenegraph/Toolbar.js
	modifié :         src/components/viewport/CameraToolbar.js
	modifié :         src/lib/EditorControls.js
	modifié :         src/lib/cameras.js
	modifié :         src/lib/raycaster.js
	modifié :         src/lib/viewport.js

but we can do that in a separate PR, I created the issue #642 for this.

@@ -0,0 +1,5 @@
{
"printWidth": 80,
Copy link
Member

Choose a reason for hiding this comment

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

Does this enforce 80 character lines? If that's the case not sure if these days makes much sense. In today's displays aspect ratio vertical space is more of a premium than horizontal

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's actually the default.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

All files are already using the 80 characters limit actually, so no real change here.

@dmarcos
Copy link
Member

dmarcos commented Nov 30, 2022

This one is ready to go? Thanks for the patience.

@vincentfretin
Copy link
Contributor Author

Yes, please merge.

@dmarcos dmarcos merged commit 37e5c6c into aframevr:master Nov 30, 2022
@vincentfretin vincentfretin deleted the code-cleanup branch December 6, 2022 16:14
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