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

added option for textToPoints() to allow separated paths #4087

Closed
wants to merge 1 commit into from

Conversation

ffd8
Copy link
Contributor

@ffd8 ffd8 commented Oct 15, 2019

Resolves #4086

Changes:
Adds additional option for textToPoints(), called separatePaths: true, which will instead export a 2D Array [paths][points] rather than only the current 1D Array [points]. An example has also been added to demonstrate how one should use it.

Screenshots of the change:
textToPoints_update

textToPoints() with 1D array (current function):
textToPoints_1d_array
textToPoints() with 2D array (proposed update):
textToPoints_2d_array

PR Checklist

  • npm run lint passes
  • [Inline documentation] is included / updated
  • [Unit tests] are included / updated
  • [Benchmarks] are included / updated

Question

Regarding the JSDoc, not sure how to deal with the fact the @return changes if requesting separatePaths? Hopefully it's clear from the description of that option and example, but maybe there's a format for suggesting a 2nd type of return if the option is set?

@welcome
Copy link

welcome bot commented Oct 15, 2019

🎉 Thanks for opening this pull request! Please check out our contributing guidelines if you haven't already. And be sure to add yourself to the list of contributors on the readme page!

@ffd8 ffd8 changed the title added option for textToPoints() to allow separated paths, resolves #4086 added option for textToPoints() to allow separated paths Oct 15, 2019
@dhowe
Copy link
Contributor

dhowe commented Oct 16, 2019

I added a couple of questions on the original ticket

@ffd8
Copy link
Contributor Author

ffd8 commented Nov 28, 2019

Temp closing this PR while awaiting approval for textToPoints() extending method... Wanted to PR a really simple fix for rect() and I guess one can't have separate PR's?

@ffd8 ffd8 closed this Nov 28, 2019
@limzykenneth
Copy link
Member

@ffd8 You can have separate PRs, you just need to have your changes on different branches on your fork.

We don't directly point to that in the contributors docs as in most cases it's not necessary in most cases and make the barrier for entry a bit higher than necessary, but it is what github usually recommends (forking then branching before making changes).

@ffd8
Copy link
Contributor Author

ffd8 commented Nov 28, 2019

@limzykenneth True - normally I'd work with separate branches, but was following the contributors docs manual a little too closely. I think it's worth suggesting people make a branch for each edit – then make a PR to the master branch from it... since any rejected/paused PR on the master branch causes tricky issues to undo it and re-sync with the official p5.js master. As I followed the suggest workflow (normally using Github GUI.. but enjoyed following manual for CLI git), I ran into the following issue: had been working on this PR – then while awaiting review, wanted to add a possible fix/extension to rect(). In order to clean up my setup, I needed to do the following:

# save changes of master to another branch
git branch new_name_for_changes

# go back to master
git checkout master

# revert master to before local changes (repeat until no longer your changes/history)
git reset --hard HEAD^

# replace local master with lastest p5.js
git pull https://github.com/processing/p5.js.git

# continue working....

# push new changed local master to own remote repo
git push --force

@limzykenneth
Copy link
Member

limzykenneth commented Nov 28, 2019

@ffd8 Yeah it may be necessary to reconsider whether we should suggest branching first.

However to fix your particular issue I would just branch from the origin HEAD. (I only use CLI but similar should be possible from GUI as well). In git log you will be able to see the commit origin HEAD is at and instead of resetting to that commit you can branch from that commit with git branch branchname <commit hash>, then new changes can be made on the branch.

To fast-forward master to latest then you may need to roll back master, that's why I usually only use and suggest using master branch for quick PRs (I'm too lazy to branch sometimes). (EDIT: Rebase actually should work well to update your local branch quite cleanly so there's no need to roll back master at all) But usually a git pull --rebase is able to replay your changes on top of the new ones from origin as long as there's no uncommitted changes.

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.

textToPoints() to allow 2D Array of points within paths for separating letters
3 participants