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

indent option #15

Merged
merged 6 commits into from
Aug 23, 2019
Merged

indent option #15

merged 6 commits into from
Aug 23, 2019
+36 −12

Conversation

rap2hpoutre
Copy link
Contributor

@rap2hpoutre rap2hpoutre commented Jul 24, 2019 •
edited by jbcarpanelli
Loading

Added indent option. Fixes #14

Capture d’écran 2019-07-24 à 11 12 46

@rap2hpoutre rap2hpoutre mentioned this pull request Jul 25, 2019
Closed
@SweetMNM
Copy link

SweetMNM commented Aug 8, 2019 •
edited
Loading

If you try to indent a multi-line text spinner everything breaks.
Try using the indent option on the spinner that would love to make some friends and see for yourself.
The problem is getLinesLength and breakText don't take the indent option into consideration.

Edit: getLinesLength does take indent into consideration the problem is with breakText.
breakText is not aware of the extra spaces which will result in the text breaking at the wrong spot.

Also multi-line strings should have all lines indented.

SweetMNM added a commit to SweetMNM/dreidels that referenced this pull request Aug 9, 2019
@SweetMNM
🎐 Add support for the 'indent' option
4df6cd4 
breakText, indentText and getLinesLength were modified to handle the indent option.

Each line in a mutli-line text spinner will be indented as well.

See:
jbcarpanelli#14
rap2hpoutre/taskz#3
jbcarpanelli#15
@jbcarpanelli
Copy link
Owner

jbcarpanelli commented Aug 9, 2019 •
edited
Loading

Hello, @rap2hpoutre! Sorry for the super late response! I just came back from vacations! I'll review this PR by the end of the weekend 🙂.

@rap2hpoutre
Copy link
Contributor Author

rap2hpoutre commented Aug 19, 2019 •
edited
Loading

I fixed the multiline problem thanks to @SweetMNM comment (in ac1a4b0)

@jcarpanelli You could now retry, it works for me (even with multi-line).

jbcarpanelli
jbcarpanelli reviewed Aug 21, 2019
View reviewed changes
Copy link
Owner

@jbcarpanelli jbcarpanelli left a comment

Choose a reason for hiding this comment

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

Hey @rap2hpoutre! Sorry for the late review, and thank you very much for tackling this! It seems to be working like a charm 💪.
Could you please add tests to this new implementation? After that, we should be ready to merge and release a new minor 😎

@rap2hpoutre
Copy link
Contributor Author

@jcarpanelli I added some tests in this commit: d515657. Let me know if it' OK for you

jbcarpanelli
jbcarpanelli approved these changes Aug 23, 2019
View reviewed changes
Copy link
Owner

@jbcarpanelli jbcarpanelli left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks again, @rap2hpoutre! 💪 🎉

@jbcarpanelli jbcarpanelli merged commit 4e4df78 into jbcarpanelli:master Aug 23, 2019
@rap2hpoutre
Copy link
Contributor Author

@jcarpanelli 👍 Thank you! When do you plan to release it?

@jbcarpanelli
Copy link
Owner

Hey @rap2hpoutre! I've just published v0.5.0 🙂

@rap2hpoutre
Copy link
Contributor Author

Thank you!!

@SweetMNM SweetMNM mentioned this pull request Sep 2, 2019
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jbcarpanelli jbcarpanelli

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

Successfully merging this pull request may close these issues.

Indent
3 participants
@rap2hpoutre @SweetMNM @jbcarpanelli