Updated RedBox screen #22242

Monte9 · 2018-11-12T02:34:11Z

Re: RedBox screen is a bit scary - Discussions and Proposals

The RedScreen was inspired by Ruby on Rails's error screen

I do see the RedBox screen could be made less jarring while still successfully displaying all the information we need.

Hence @jamonholmgren came up with the idea that only the header & footer of the RedBox screen could be red. This makes the content a bit more readable as well as makes the screen a little less intimidating.

Also @frantic made the suggestion that since the bottom buttons are not as important, they don't need to stand out. Hence only the header of the RedBox screen which displays the error is made red.

Screenshots:

iOS

Android:

Test Plan:

Use RNTester iOS & Android app to generate an error and test the updated error screen.

Changelog:

[General] [Changed] - RedBox Screen has new dark mode design to improve readability.

facebook-github-bot · 2018-11-12T02:34:19Z

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need the corporate CLA signed.

If you have received this in error or have any questions, please contact us at cla@fb.com. Thanks!

pull-bot · 2018-11-12T02:35:44Z

	Warnings
⚠️	📋 Release Notes - This PR appears to be missing Release Notes.

Generated by 🚫 dangerJS

facebook-github-bot · 2018-11-12T02:39:17Z

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

axe-fb · 2018-11-12T03:08:33Z

This looks nice !!

kelset · 2018-11-12T17:07:09Z

This looks awesome, thanks for the PR 👏

Titozzz · 2018-11-12T17:19:11Z

Shouldn't the bottom text on iOS be moved up a bit, It was not designed with IphoneX in mind and it could be annoying to use the buttons?

Look way better tho, great work 👍

TheSavior · 2018-11-12T18:31:25Z

This is great.

Can we put the hot key under the title in the bottom bar on iOS? More like Androids. That should help keep the buttons from running into each other and the edges.

frantic · 2018-11-12T19:34:15Z

The author of the original Red Box here :)

This is awesome! A couple of minor suggestions:

Make the buttons at the bottom consistent between iOS and Android (specifically shortcuts placement as suggested by @TheSavior)
Can we make the bottom part black too? With two reds in top and bottom the screen looks almost symmetrical but not quite. Also the buttons aren't as important as the error message.
Are the subtitles intentionally red-tinted? Can we make them more neutral (grey-ish)?
Add extra padding on iPhone X
Changelog: use a more positive phrasing, e.g. "RedBox has new dark mode design to improve readability"

I understand that addressing this is a little outside of the initial scope of just changing the colors, but I truly believe all this little thing will add up to a much better error message experience.

Monte9 · 2018-11-14T00:17:09Z

@Titozzz, @TheSavior that's a good idea. Will update iOS so the layout of the buttons are more consistent with Android.

@frantic appreciate your feedback on this PR! I really like your suggestions and agree that these would be good to work on as part of this PR to improve the overall error screen dx.

In regards to updating the bottom bar to be black.. I agree with you in principle about the bottom buttons not being as important as the error message at the top. I'll give it a shot and will post a screenshot here so we can compare the two and see which one we like better.

Also, the reason the subtitle is red-tinted is so it's easier to read on a black background. But I'll try a shade of gray that might contrast well and see if that works too.

And I really like the way you phrased the changelog. Will end up using that instead. Thanks!

I'll work on what we talked about above and follow up in a couple days. 🏃

henrikra · 2018-11-14T08:03:23Z

Hmm buttons are too close to bottom of the iPhone X 🤔

… iPhoneX

Monte9 · 2018-11-17T20:02:28Z

Make the buttons at the bottom consistent between iOS and Android
Make the bottom part black too
Make the subtitles more neutral (grey-ish)?
New color: #B3B3B3
Add extra padding on iPhone X
Added conditional SafeAreaView
Changelog: RedBox has new dark mode design to improve readability.

/cc @frantic please take a look and let me know if you have any other suggestions.

Updated Screenshots

iOS:

Android:

Monte9 · 2018-11-17T20:36:40Z

React/Modules/RCTRedBox.m

+        CGSizeEqualToSize(screenSize, iPhoneXMaxScreenSize) ||
+        CGSizeEqualToSize(screenSize, iPhoneXRScreenSize);
+}
+


@kelset this is not ideal as the code is redundant with RCTDeviceInfo.

RCTDeviceInfo has a func called RCTIsIPhoneX but since I am not very proficient in Obj-C, I couldn't figure out how to reuse it. Any suggestions on how to reuse/improve this code would be appreciated!

Yeah there's no easy way to reuse that function unless we extract it out to a util module.

I think this could work: safeAreaInsets: according to the docs, if the rootView is part of the view hierarchy, this value should return the extra padding we need to add to our UI.

@shergin what's the best way to do this?

@Monte9 do you mind following up on @frantic's last comment? I think that's the only remaining blocker to getting this shipped :)

@cpojer Yes, will update it tonight. Was waiting for @shergin to respond. 👍

@frantic quick follow up: when I try to use the safeAreaInsets on the rootview I get 0 on an iPhoneX.

NSLog(@"safeArea: %f", rootView.safeAreaInsets.bottom); > 2018-12-13 14:42:24.955719-0600 RNTester[37397:1022268] safeArea: 0.000000

I looked up a few issues on stack overflow and I figure out that I need to do this instead:

[UIApplication sharedApplication].delegate.window.safeAreaInsets.bottom;

Hence I came up with this which is pretty abstracted and much better than what I had before. Thanks for the suggestion. 👍

- (NSInteger)bottomSafeViewHeight { if (@available(iOS 11.0, *)) { return [UIApplication sharedApplication].delegate.window.safeAreaInsets.bottom; } else { return 0; } }

Monte9 · 2018-11-17T20:41:29Z

React/Modules/RCTRedBox.m

-        [copyButton setTitleColor:[UIColor colorWithWhite:1 alpha:0.5] forState:UIControlStateNormal];
-        [copyButton setTitleColor:[UIColor whiteColor] forState:UIControlStateHighlighted];
+        [copyButton setTitleColor:[UIColor whiteColor] forState:UIControlStateNormal];
+        [copyButton setTitleColor:[UIColor colorWithWhite:1 alpha:0.5] forState:UIControlStateHighlighted];
        [copyButton addTarget:self action:@selector(copyStack) forControlEvents:UIControlEventTouchUpInside];


@kelset Also ideally all those buttons should be in a func that just takes a param and returns a UIButton.

This would clean up the code a lot, but I haven’t done Obj-C in a while. Let me know if that makes sense, and if something like would work.

I think it's okay for now. Refactoring the buttons is probably outside of the scope of this change

frantic

@Monte9 thank you so much for the screenshots, they make it so much easier to review this change!

I've added a few comments inline. The biggest one is detecting iPhone X, we could try to reuse the function from RCTDeviceInfo but would be even better if we could use iOS built-in APIs. I'll ping @shergin for suggestions.

Another thing I'd like to ask you is to change this line from UITableViewCellStyleSubtitle to UITableViewCellStyleDefault, this will make the error text properly vertically aligned.

frantic · 2018-11-18T18:57:04Z

React/Modules/RCTRedBox.m

-        [copyButton setTitleColor:[UIColor colorWithWhite:1 alpha:0.5] forState:UIControlStateNormal];
-        [copyButton setTitleColor:[UIColor whiteColor] forState:UIControlStateHighlighted];
+        [copyButton setTitleColor:[UIColor whiteColor] forState:UIControlStateNormal];
+        [copyButton setTitleColor:[UIColor colorWithWhite:1 alpha:0.5] forState:UIControlStateHighlighted];
        [copyButton addTarget:self action:@selector(copyStack) forControlEvents:UIControlEventTouchUpInside];


I think it's okay for now. Refactoring the buttons is probably outside of the scope of this change

frantic · 2018-11-18T18:59:15Z

React/Modules/RCTRedBox.m


        [rootView addSubview:dismissButton];
        [rootView addSubview:reloadButton];
        [rootView addSubview:copyButton];
        [rootView addSubview:extraButton];
+
+        if ([self isIPhoneX]) {
+            [rootView addSubview:bottomSafeView];


Move lines 160-162 here, so we don't create the UIView in case we don't need it

👍 please do

@frantic good point. Will update.

frantic · 2018-11-18T19:06:05Z

React/Modules/RCTRedBox.m

+        CGSizeEqualToSize(screenSize, iPhoneXMaxScreenSize) ||
+        CGSizeEqualToSize(screenSize, iPhoneXRScreenSize);
+}
+


Yeah there's no easy way to reuse that function unless we extract it out to a util module.

I think this could work: safeAreaInsets: according to the docs, if the rootView is part of the view hierarchy, this value should return the extra padding we need to add to our UI.

@shergin what's the best way to do this?

frantic · 2018-11-18T19:08:03Z

React/Modules/RCTRedBox.m

+
+        UIView *topBorderExtraButton = [[UIView alloc] initWithFrame:CGRectMake(0, 0, extraButton.frame.size.width, 1)];
+        topBorderExtraButton.backgroundColor = [UIColor colorWithRed:0.70 green:0.70 blue:0.70 alpha:1.0];
+        [extraButton addSubview:topBorderExtraButton];


Instead of creating top border for each button, can we use one full-width view and add it to the rootView directly?

Yeah, that does sound better. Updated. Cleanup up 20 lines. 💯

jamonholmgren · 2018-11-19T03:24:07Z

I really <3 this new design, and am impressed by the community effort here!

henrikra · 2018-11-19T05:33:53Z

Red top bar should expand to the top on iphonex

radeno · 2018-11-20T10:20:40Z

@Monte9 awesome work! No more flash bangs. Eyes saver. Hope one day YellowBox will be same usable and readable.

hramos · 2018-12-13T20:44:35Z

@radeno YellowBox was updated this past summer, and it should be easier to contribute to. It's out of the scope of this PR, but feel free to open an issue describing how the design could be improved. That way we won't lose track of the issue once this PR is merged.

hramos

Looks like there's some feedback from Alex that's not addressed yet. Will hold off on importing for now.

Monte9 · 2018-12-13T21:13:06Z

@frantic thanks for all the feedback. Def cleaned up & improved the code. I have addressed the feedback. Can you please take a look at it again?

@hramos @cpojer this PR should be ready to go once @frantic approves. I'd love to get it out in one of the v0.58 RC's so that we can get some feedback from the community on this improved DX.

facebook-github-bot

@cpojer has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

Monte9 · 2019-01-02T23:59:21Z

Ping @frantic

Monte9 · 2019-01-08T20:01:25Z

@hramos @cpojer what else needs to be done in order to get this PR merged in? I have addressed @frantic's comments although he seems to be afk.

I see that @cpojer imported this PR.. but not sure what that means? @kelset Can you give me a tl;dr of the lifecycle of a RN PR. (afaik it gets imported into FB's version of RN and is used internally first then it's merged into open source?)

kelset · 2019-01-09T09:51:40Z

Basically, the tl;dr (or at least my understanding of it) is that once it's imported, someone internally need to 'manually merge it' into the FB monorepo. Once that's done, the bot will close the PR and a commit will appear on master.

So it's basically in the internal phase right now, which means that it's probably getting reviewed & tested. It may take a bit longer since probably the team is "fully operative" starting next week.

hramos · 2019-01-10T19:52:38Z

What @kelset said. I just got back from being away for a few weeks on holiday break - I'll take a look at the imported diff ASAP :)

hramos

Approved. Christoph is out, I'll let him ship this when he's back.

react-native-bot · 2019-01-15T14:35:04Z

@Monte9 merged commit 7fbccde into facebook:master.

Summary: [Re: RedBox screen is a bit scary - Discussions and Proposals](react-native-community/discussions-and-proposals#42) Per hramos: > The RedScreen was inspired by Ruby on Rails's error screen > I do see the RedBox screen could be made less jarring while still successfully displaying all the information we need. Hence jamonholmgren came up with the idea that only the header & footer of the RedBox screen could be red. This makes the content a bit more readable as well as makes the screen a little less intimidating. Also frantic made the suggestion that since the bottom buttons are not as important, they don't need to stand out. Hence only the header of the RedBox screen which displays the error is made red. Screenshots: ---------- <div style="flex-direction: row"> <img width="325" alt="orginal" src="https://user-images.githubusercontent.com/7840686/48322916-b4958b80-e5de-11e8-9276-33378d1b41c5.png"> <img width="320" alt="redbox_v2_ios" src="https://user-images.githubusercontent.com/7840686/48665300-cce32b80-ea60-11e8-8e8f-88f74bad30ca.png"> </div> <div style="flex-direction: row"> <img width="300" alt="original_android" src="https://user-images.githubusercontent.com/7840686/48322958-d5f67780-e5de-11e8-891c-1b20bd00e67b.png"> <img width="300" alt="redbox_v2_android" src="https://user-images.githubusercontent.com/7840686/48665312-f13f0800-ea60-11e8-9fb6-47e03c809789.png"> </div> Pull Request resolved: #22242 Reviewed By: hramos Differential Revision: D13564287 Pulled By: cpojer fbshipit-source-id: fcb6ba5e20d863f4b957d20f3787f5b7a365bfdb

Summary: [Re: RedBox screen is a bit scary - Discussions and Proposals](react-native-community/discussions-and-proposals#42) Per hramos: > The RedScreen was inspired by Ruby on Rails's error screen > I do see the RedBox screen could be made less jarring while still successfully displaying all the information we need. Hence jamonholmgren came up with the idea that only the header & footer of the RedBox screen could be red. This makes the content a bit more readable as well as makes the screen a little less intimidating. Also frantic made the suggestion that since the bottom buttons are not as important, they don't need to stand out. Hence only the header of the RedBox screen which displays the error is made red. Screenshots: ---------- <div style="flex-direction: row"> <img width="325" alt="orginal" src="https://user-images.githubusercontent.com/7840686/48322916-b4958b80-e5de-11e8-9276-33378d1b41c5.png"> <img width="320" alt="redbox_v2_ios" src="https://user-images.githubusercontent.com/7840686/48665300-cce32b80-ea60-11e8-8e8f-88f74bad30ca.png"> </div> <div style="flex-direction: row"> <img width="300" alt="original_android" src="https://user-images.githubusercontent.com/7840686/48322958-d5f67780-e5de-11e8-891c-1b20bd00e67b.png"> <img width="300" alt="redbox_v2_android" src="https://user-images.githubusercontent.com/7840686/48665312-f13f0800-ea60-11e8-9fb6-47e03c809789.png"> </div> Pull Request resolved: facebook#22242 Reviewed By: hramos Differential Revision: D13564287 Pulled By: cpojer fbshipit-source-id: fcb6ba5e20d863f4b957d20f3787f5b7a365bfdb

Summary: [Re: RedBox screen is a bit scary - Discussions and Proposals](react-native-community/discussions-and-proposals#42) Per hramos: > The RedScreen was inspired by Ruby on Rails's error screen > I do see the RedBox screen could be made less jarring while still successfully displaying all the information we need. Hence jamonholmgren came up with the idea that only the header & footer of the RedBox screen could be red. This makes the content a bit more readable as well as makes the screen a little less intimidating. Also frantic made the suggestion that since the bottom buttons are not as important, they don't need to stand out. Hence only the header of the RedBox screen which displays the error is made red. Screenshots: ---------- <div style="flex-direction: row"> <img width="325" alt="orginal" src="https://user-images.githubusercontent.com/7840686/48322916-b4958b80-e5de-11e8-9276-33378d1b41c5.png"> <img width="320" alt="redbox_v2_ios" src="https://user-images.githubusercontent.com/7840686/48665300-cce32b80-ea60-11e8-8e8f-88f74bad30ca.png"> </div> <div style="flex-direction: row"> <img width="300" alt="original_android" src="https://user-images.githubusercontent.com/7840686/48322958-d5f67780-e5de-11e8-891c-1b20bd00e67b.png"> <img width="300" alt="redbox_v2_android" src="https://user-images.githubusercontent.com/7840686/48665312-f13f0800-ea60-11e8-9fb6-47e03c809789.png"> </div> Pull Request resolved: facebook/react-native#22242 Reviewed By: hramos Differential Revision: D13564287 Pulled By: cpojer fbshipit-source-id: fcb6ba5e20d863f4b957d20f3787f5b7a365bfdb

Monte9 added 2 commits November 11, 2018 17:36

updated ios red box - looks better

737b04f

updated android red box - much better

b203c9d

Monte9 requested a review from shergin as a code owner November 12, 2018 02:34

facebook-github-bot added the ✅pr adds tests label Nov 12, 2018

Monte9 mentioned this pull request Nov 12, 2018

RedBox screen is a bit scary react-native-community/discussions-and-proposals#42

Closed

facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Nov 12, 2018

hramos removed the ✅pr adds tests label Nov 14, 2018

address PR feedback - bottom bar made black; added bottomSafeArea for…

1e13f37

… iPhoneX

Monte9 commented Nov 17, 2018

View reviewed changes

fixed typo: isIPhoneX

d8af495

Monte9 commented Nov 17, 2018

View reviewed changes

frantic suggested changes Nov 18, 2018

View reviewed changes

Monte9 mentioned this pull request Dec 12, 2018

0.58.x Discussion react-native-community/releases#41

Closed

gengjiawen approved these changes Dec 13, 2018

View reviewed changes

hramos suggested changes Dec 13, 2018

View reviewed changes

addressed PR feedback

5fa32b8

facebook-github-bot reviewed Jan 2, 2019

View reviewed changes

hramos approved these changes Jan 10, 2019

View reviewed changes

facebook-github-bot closed this in 7fbccde Jan 15, 2019

facebook locked as resolved and limited conversation to collaborators Jan 15, 2019

react-native-bot added the Merged This PR has been merged. label Jan 15, 2019

Updated RedBox screen #22242

Updated RedBox screen #22242

Conversation

Monte9 commented Nov 12, 2018 • edited

Screenshots:

iOS

Android:

Test Plan:

Changelog:

facebook-github-bot commented Nov 12, 2018

pull-bot commented Nov 12, 2018

facebook-github-bot commented Nov 12, 2018

axe-fb commented Nov 12, 2018

kelset commented Nov 12, 2018

Titozzz commented Nov 12, 2018

TheSavior commented Nov 12, 2018

frantic commented Nov 12, 2018

Monte9 commented Nov 14, 2018

henrikra commented Nov 14, 2018

Monte9 commented Nov 17, 2018 • edited

Updated Screenshots

iOS:

Android:

Monte9 Nov 17, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Monte9 Dec 10, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

frantic left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Monte9 Dec 10, 2018 • edited

Choose a reason for hiding this comment

jamonholmgren commented Nov 19, 2018

henrikra commented Nov 19, 2018

radeno commented Nov 20, 2018

hramos commented Dec 13, 2018

hramos left a comment

Choose a reason for hiding this comment

Monte9 commented Dec 13, 2018 • edited

facebook-github-bot left a comment

Choose a reason for hiding this comment

Monte9 commented Jan 2, 2019

Monte9 commented Jan 8, 2019

kelset commented Jan 9, 2019

hramos commented Jan 10, 2019

hramos left a comment

Choose a reason for hiding this comment

react-native-bot commented Jan 15, 2019

Monte9 commented Nov 12, 2018 •

edited

Monte9 commented Nov 17, 2018 •

edited

Monte9 Nov 17, 2018 •

edited

Monte9 Dec 10, 2018 •

edited

Monte9 Dec 10, 2018 •

edited

Monte9 commented Dec 13, 2018 •

edited