Skip to content

do not show alert if sp help text is a blank string#4487

Merged
mitchellhenke merged 1 commit intomasterfrom
mitchellhenke/fix-empty-sp-alert
Dec 4, 2020
Merged

do not show alert if sp help text is a blank string#4487
mitchellhenke merged 1 commit intomasterfrom
mitchellhenke/fix-empty-sp-alert

Conversation

@mitchellhenke
Copy link
Contributor

Potentially introduced by #4418 ?

image

@mitchellhenke mitchellhenke requested review from aduth and orenyk December 4, 2020 16:01
Copy link
Contributor

@zachmargolis zachmargolis left a comment

Choose a reason for hiding this comment

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

LGTM, minor comments

Copy link
Contributor

Choose a reason for hiding this comment

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

let's fix the comment? it "is nil" maybe? or "it doesn't show anything"?

Copy link
Contributor

Choose a reason for hiding this comment

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

and lets fix the one on line 69 above too?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh wow, didn't even notice. good catch, updated

@mitchellhenke mitchellhenke force-pushed the mitchellhenke/fix-empty-sp-alert branch from 3d5ddc9 to 3ea9c87 Compare December 4, 2020 16:06
Copy link
Contributor

@aduth aduth left a comment

Choose a reason for hiding this comment

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

Copy link
Contributor

@orenyk orenyk left a comment

Choose a reason for hiding this comment

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

:shipit:

@mitchellhenke mitchellhenke merged commit fe4f643 into master Dec 4, 2020
@mitchellhenke mitchellhenke deleted the mitchellhenke/fix-empty-sp-alert branch December 4, 2020 16:19
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.

4 participants