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.

Sign up for GitHub

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

Jump to bottom

Src/extras/core: [remake] move to es6 classes #20656

Closed
wants to merge 6 commits into from
Closed

Src/extras/core: [remake] move to es6 classes #20656

wants to merge 6 commits into from

Conversation

DefinitelyMaybe
Copy link
Contributor

@DefinitelyMaybe DefinitelyMaybe commented Nov 9, 2020 •
edited
Loading

Related issue: #20143

Description

Remake of related issue.

PR also includes change to .gitignore, which added some .ps1 scripts when I ran npm i --prefix test

@DefinitelyMaybe DefinitelyMaybe changed the title Src/extras/core remake] move to es6 classes Src/extras/core: [remake] move to es6 classes Nov 9, 2020
@DefinitelyMaybe DefinitelyMaybe marked this pull request as draft November 9, 2020 21:15
@DefinitelyMaybe DefinitelyMaybe marked this pull request as ready for review November 9, 2020 21:19
@DefinitelyMaybe
Copy link
Contributor Author

DefinitelyMaybe commented Nov 9, 2020 •
edited
Loading

resolved conflicts from related PR

@Mugen87
Copy link
Collaborator

Mugen87 commented Nov 9, 2020 •
edited
Loading

I think it's more safe if you mark your ES6 class PRs as Draft PRs. Most of these can't be merged without adjusting the respective example code.

@DefinitelyMaybe DefinitelyMaybe mentioned this pull request Nov 9, 2020
Closed
43 tasks
@DefinitelyMaybe
Copy link
Contributor Author

DefinitelyMaybe commented Nov 9, 2020 •
edited
Loading

I moved to draft earlier because things weren't checking out but, yes, should've done that in the first instance after initial PR tests failed.

What was the respective example code that needed to be changed for this PR? Could I simply include that adjustment within this PR?

mrdoob
mrdoob reviewed Feb 4, 2021
View reviewed changes
.gitignore
@@ -17,7 +17,8 @@ test/*
!test/*/
!test/*.*
test/*.cmd
test/*.ps1
Copy link
Owner

@mrdoob mrdoob Feb 4, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

What are ps1 files?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Windows PowerShell scripts.

mrdoob
mrdoob reviewed Feb 4, 2021
View reviewed changes
src/extras/core/Font.js

function createPaths( text, size, data ) {
Copy link
Owner

Choose a reason for hiding this comment

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

Why not keep this as a function outside the class?

@mrdoob
Copy link
Owner

mrdoob commented Feb 4, 2021

This PR seems to be almost good to go. The only issue is the createPaths change.

@mrdoob mrdoob added this to the r126 milestone Feb 4, 2021
@DefinitelyMaybe
Copy link
Contributor Author

This PR is too old. Someone else has already changed font.js to a class and left function createPaths(... outside the class. The strangeness from test/*.ps1 is from new commits.

I will remake again with the other changes I made.

DefinitelyMaybe added a commit to DefinitelyMaybe/three.js that referenced this pull request Feb 4, 2021
@DefinitelyMaybe
old changes from PR mrdoob#20656 minus font.js
9ad8182
@DefinitelyMaybe DefinitelyMaybe mentioned this pull request Feb 4, 2021
Merged
@mrdoob mrdoob removed this from the r126 milestone Feb 4, 2021
@DefinitelyMaybe DefinitelyMaybe deleted the src/extras/core---remake]-move-to-es6-classes branch February 6, 2021 09:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mrdoob mrdoob mrdoob left review comments

@looeee looeee looeee left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@DefinitelyMaybe @Mugen87 @mrdoob @looeee