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 glitches #24

Merged
merged 4 commits into from
Oct 6, 2016
Merged

Fix glitches #24

merged 4 commits into from
Oct 6, 2016

Conversation

mischah
Copy link
Member

@mischah mischah commented Oct 5, 2016

Yo @yeoman/yosay,

This PR fixes two (not so edgy) edge case issues I introcuded with #23.

First of all let me say how sorry and ashamed I am for creating these bugs 😖

Bug No. 1

The overflowing of messages that are too long is producing corrupted output of speech bubble when using the maxLength option.

broken-overflow-with-maxlength

This is fixed with:

4ed8662 Fix overflowing messages when using maxLength

See screenshot:

fixed-overflow-with-maxlength

Bug No. 2

Calling yosay more than once within a script adapting topOffset causes corrupted output of the latter speech bubbles.

broken-multiple-spechbubbles

This is fixed with:

03a990a Fix problem with top offset and left padding …

See screenshot:

fixed-multiple-spechbubbles

## Conclusion

As said before I am terribly sorry to messed this up with the introduction of the speech bubble tip.

It was caused by not thoroughly hand testing and the auto generating of test fixtures 😧

I made sure to hand test the fixes of this PR by with this node file:

const yosay = require('yosay');
const chalk = require('chalk');

console.log(yosay('Hello, and welcome to my fantastic generator full of whimsy and bubble gum!'));

console.log(yosay('Hi'));

console.log(yosay('Welcome to Yeoman, ladies and gentlemen!'));

console.log(yosay('Hi', {maxLength: 8}));

console.log(yosay('Hello, buddy!', {maxLength: 4}));

console.log(yosay(chalk.red.bgWhite('Hi')));

console.log(yosay(chalk.red.bgWhite('Hi') + ' there, sir!'));

console.log(yosay(chalk.red.bgWhite('Hi') + ' there, sir! ' + chalk.bgBlue.white('you are looking') + ' swell today!'));

console.log(yosay('first line\nsecond line\n\nfourth line'));

console.log(yosay('项目可以更新了'));

console.log(yosay('iloveunicornsiloveunicornsiloveunicornsiloveunicornsiloveunicornsiloveunicorns'));


console.log(yosay('Lie on your belly and purr when you are asleep shove bum in owner’s face like camera lens. Cough furball.', {maxLength: 11}));
console.log(yosay('Lie on your belly and purr when you are asleep shove bum in owner’s face like camera lens. Cough furball. Cough furball. Cough furball. Cough furball. Cough furball. Cough furball. Cough furball. Cough furball. Cough furball. Cough furball. Cough furball. Cough furball. Cough furball.'));
console.log(yosay('Lie on your belly and purr when you are asleep shove bum in owner’s face like camera lens. Cough furball. Cough furball. Cough furball. Cough furball. Cough furball. Cough furball. Cough furball. Cough furball. Cough furball. Cough furball. Cough furball. Cough furball. Cough furball.', {maxLength: 11}));
console.log(yosay('Lie on your belly and purr when you are asleep shove bum in owner’s face like camera lens. Cough furball. Cough furball. Cough furball. Cough furball. Cough furball. Cough furball. Cough furball. Cough furball. Cough furball. Cough furball. Cough furball. Cough furball. Cough furball.', {maxLength: 26}));

console.log(yosay(
    'That’s it. Feel free to fire up the server with ' +
    chalk.green('`npm run start:dev`') +
    'or use our subgenerator to create endpoints.'
));

console.log(yosay ('That’s it. Feel free to fire up the server with `npm run start:dev` or use our subgenerator to create endpoints.'));

console.log(yosay(
    'That’s it. Feel free to fire up the server with ' +
    chalk.green('`npm run start:dev`') + '.'
));

console.log(yosay(
    'That’s it. Feel free to fire up the server with ' +
    '`npm run start:dev`.'
));

And this bash file:

yosay 'Hi' && 
yosay 'Welcome to Yeoman, ladies and gentlemen!' && 
yosay 'Hi' --maxLength 8 &&
yosay 'Hello, buddy!' --maxLength 4 && 
yosay '项目可以更新了' && 
yosay 'iloveunicornsiloveunicornsiloveunicornsiloveunicornsiloveunicornsiloveunicorns' &&
yosay 'Lie on your belly and purr when you are asleep shove bum in owner’s face like camera lens. Cough furball.' --maxLength 11 && 
yosay 'Lie on your belly and purr when you are asleep shove bum in owner’s face like camera lens. Cough furball. Cough furball. Cough furball. Cough furball. Cough furball. Cough furball. Cough furball.' && 
yosay 'Lie on your belly and purr when you are asleep shove bum in owner’s face like camera lens. Cough furball. Cough furball. Cough furball. Cough furball. Cough furball. Cough furball. Cough furball. Cough furball. Cough furball. Cough furball. Cough furball. Cough furball. Cough furball.' --maxLength 26 && 
yosay 'That’s it. Feel free to fire up the server with `npm run start:dev`.'

… in case you use yosay repeatedly to generate multiple speech bubbles within one script.
@mischah
Copy link
Member Author

mischah commented Oct 5, 2016

I would be super happy if @sindresorhus could review that as soon as possible (He’s the one who helped me with #23)

Cause I would love to merge that into master and prepare a release as soon as possible to get these bugfixes out on the street.

@mischah
Copy link
Member Author

mischah commented Oct 5, 2016

You want me to squash the 3 commits before that PR lands into master?

@sindresorhus
Copy link
Member

No worries at all. The code here is very complex.

Could you add the above Node.js script as manual-test.js to the repo? So we can run the manual tests to verify whenever.

@sindresorhus
Copy link
Member

You want me to squash the 3 commits before that PR lands into master?

No need. GitHub has a squash and merge button now.

@sindresorhus sindresorhus merged commit d56182d into master Oct 6, 2016
@sindresorhus sindresorhus deleted the fix-glitches branch October 6, 2016 08:51
@mischah
Copy link
Member Author

mischah commented Oct 6, 2016

I’ve just added the manual-test.jsfile.
Let me know if there is anything else I can do.

Otherwise I would:

  1. Squash and merge
  2. Bump version
  3. Tag Release
  4. Add Release notes to the Release tab here on Github
  5. Ping to npm publish that thing.

@sindresorhus
Copy link
Member

Awesome. Thanks for fixing :)

@mischah
Copy link
Member Author

mischah commented Oct 6, 2016

opps. you’ve done that while I was typing.

@sindresorhus
Copy link
Member

Published.

@mischah Could you do 4.? I'm out of time.

@mischah
Copy link
Member Author

mischah commented Oct 6, 2016

You were too fast.
Build fails in older node versions because I used const in manual-test.js 😵

@sindresorhus
Copy link
Member

Already fixed

@mischah
Copy link
Member Author

mischah commented Oct 6, 2016

ah. okay. man you’re fast like a 🚀

@mischah
Copy link
Member Author

mischah commented Oct 6, 2016

Add Release notes to the Release tab here on Github

Done. Thanks for everything. Highly appreciated what you are doing 💖

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants