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

Change isSingeLine to support null or undefined text #1384

Merged
merged 3 commits into from
Aug 20, 2018

Conversation

abendafb
Copy link
Contributor

With: @lsaddan

This happened when we tried connecting hydrogen to a tweaked Jupiter server.
It worked nicely with this change.

This happened when we tried connecting hydrogen to a tweaked Jupiter server.
It worked nicely with this change.
@BenRussert
Copy link
Member

Hi, thanks for the PR. I don't follow what you mean, could you clarify what the issue is and how this will fix it?

Copy link
Member

@BenRussert BenRussert left a comment

Choose a reason for hiding this comment

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

Sounds like you found a bug and fixed it, that is pretty cool in my book! I'd love if you would comment on what you found so we know!

Let me know if you want to add that flow type from my comment otherwise I will probably just merge this.

Thank you, and congrats on your first PR!

@@ -57,7 +57,7 @@ export function reduceOutputs(
export function isSingeLine(text: string, availableSpace: number) {
Copy link
Member

Choose a reason for hiding this comment

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

Since you have discovered a bug that allows undefined values to sneak in, we should change this text: string to a flow maybe type, which looks like this: text: ?string.

The ?string type will tip flow off that it is possible for the argument text to be undefined. Then flow will warn us that we need to handle the case where text === undefined.

@abendafb
Copy link
Contributor Author

Fixing null case as well

@abendafb abendafb changed the title Check text for undefined Change isSingeLine to support null or undefined text Aug 19, 2018
@abendafb
Copy link
Contributor Author

BTW, is it isSingeLine on purpose?

@BenRussert BenRussert merged commit 3291db2 into nteract:master Aug 20, 2018
@BenRussert
Copy link
Member

Thanks @abendafb!

BTW, is it isSingeLine on purpose?

No, probably not.

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.

2 participants