Skip to content

Conversation

@asklar
Copy link
Member

@asklar asklar commented Apr 29, 2020

Fixes #1039

Microsoft Reviewers: Open in CodeFlow

@asklar asklar requested a review from a team as a code owner April 29, 2020 10:00
@asklar asklar added the AutoMerge Causes a PR to be automatically merged once all requirements are passed (label drives bot activity) label Apr 29, 2020
@ghost
Copy link

ghost commented Apr 29, 2020

Hello @asklar!

Because this pull request has the AutoMerge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

Do note that I've been instructed to only help merge pull requests of this repository that have been opened for at least 60 minutes. No worries though, I will be back when the time is right! 😉

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

#ifdef DEBUG
else if (propertyValue.isNull()) {
// Log error, must have a color
YellowBox("IconShadowNode - color property must be non-null");
Copy link
Contributor

Choose a reason for hiding this comment

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

Generally Facebook only does this warning once per instance. You wouldn't want your app spammed with this error.
Unfortunately their warnOnce API isn't exposed to native. But then again, its probably not hard to do something on the native side to only do this once.

Copy link
Member Author

@asklar asklar Apr 29, 2020

Choose a reason for hiding this comment

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

@acoates-ms YellowBox already aggregates/coalesces like warnings into one and you just get one warning per message, with a badge for the # of times it has hit.
image

Copy link
Contributor

Choose a reason for hiding this comment

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

I believe that it only does that if it matches the last error. So once we have multiple of these it would get very spammy.

Copy link
Member Author

Choose a reason for hiding this comment

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

@acoates-ms confirmed that when I get the 53 test ones, then go to another page that shows a yellow box, then go to the 53 test yellowbox, the test yellowbox correctly adds those up and brings it to the top:

image

Copy link
Member

@vmoroz vmoroz left a comment

Choose a reason for hiding this comment

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

:shipit:

@ghost ghost merged commit 15ad43e into microsoft:master Apr 30, 2020
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

AutoMerge Causes a PR to be automatically merged once all requirements are passed (label drives bot activity)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Implement native mechanism for invoking the yellowbox (console.warn)

3 participants