Add Modal property overlay color

[7thPorter]

Full disclosure: This feature was originally suggested here in 2018, but no pull request was made and the issue was closed. I can’t take credit for the idea originally, but since the original poster’s GitHub account no longer exists for me to ask permission, and the issue in question persists through today, I decided to take a shot at a fix. For reference, another attempt was made to fix this same issue here, but the request was closed by the requester, rather than merged.

Summary

Currently, when a developer sets the transparency prop of a Modal component to false, a white page is rendered “behind” the modal. This effect is hardcoded. This small feature allows developers to utilize a new prop, called overlayColor, to change this background color to suit their needs. The value of overlayColor will be a string, and may include anything that could be entered as a valid backgroundColor style.

In order to facilitate possible use of transparency values in the overlayColor string (i.e. rgba values), the presentationStyle prop logic has been modified to make “overFullScreen” the default value of that prop when overlayColor is utilized. Without making this change to the presentationStyle prop, color values with transparency cannot be rendered properly on iOS.

Changelog

[General] [Added] - Add overlayColor property to Modal component

Test Plan

npm test result: When testing, I first ran the test on the main branch as a control group and received 196 failed tests of 2845 total. Not sure what causes those failures, but after implementing changes on the modal-overlayColor branch and running npm test again, the number of failed tests did not increase.

npm run flow result: The flow test showed that I hadn’t properly considered all the pieces of the Modal component.

First, and in particular, checking for the value of overlayColor as a string returned a “sketchy-null-string” error. Investigation led me to the Text component, where a similarly-functioning feature, the selectionColor prop, imports processColor from the StyleSheet component, and passes the input string through that function. Doing so in my code eliminated the sketchy-null-string error, but reveals a “sketchy-null-number” error. If possible, I’d appreciate some guidance on why this is happening, and how to resolve it, since I had never used flow before attempting these tests. In the end, I settled on a solution that works in my test code, but still gives me a couple "sketchy-null-string" error. I don’t feel like this is necessarily the most optimized solution, so if there’s a better way to approach this, I’d be eager to hear it.

Second, it showed that I needed to edit the RCTModalHostViewNativeComponent file to reflect changes made in Modal.js. Again, I relied on the TextNativeComponent file for usage of the selectionColor prop as a guide.

Usage Example

Default Behavior:

With overlayColor Enabled:

Note 1: Allowing the use of transparency values in the overlayColor prop when the transparent prop is set to false may be a bit misleading. There's probably a better solution available to remove any possible confusion, but I've taken to considering transparent to mean "true transparency", whereas the rgba value represented in the screenshot is only a partial transparency.

Note 2: Because of the behavior of the Modal component, setting the transparent prop to false results in an unaesthetic animation in the background color if the animationType property is set to "slide". This doesn't fall within the scope of this pull request, but it's probably worth noting for developers so they can expect that behavior.

[analysis-bot]

Platform	Engine	Arch	Size (bytes)	Diff
ios	-	universal	n/a	--

Base commit: 0a517ae
Branch: main

[charlesbdudley]

Thanks for this contribution! Could you take a look at the eslint and flowtype errors?

[7thPorter]

Hi @charlesbdudley this is what I was looking for guidance with, actually. Here are the results of running the flow test:

Error ---------------------------------------------------------------------------------- Libraries/Modal/Modal.js:243:13

Sketchy null check on string [1] which is potentially an empty string. Perhaps you meant to check for null or
undefined [2]? [sketchy-null-string]

 [2][1]  96|   overlayColor?: ?string,
           :
        240|       backgroundColor:
        241|         this.props.transparent === true
        242|           ? 'transparent'
        243|           : this.props.overlayColor
        244|           ? this.props.overlayColor
        245|           : 'white',
        246|     };

I don't have any experience with flow outside of running these tests, so I'm not quite sure what to do about this sketchy-null-string error. I tried my best to eliminate the errors by performing null checks (as on lines 234-237 of the Modal.js file in the PR), but when I worked this last bit of code to finally resolve the null-string error, it made the overlayColor prop non-functional, so I was left with that last error. Is there a good way to address this?

I'm also not sure I installed all of my lint packages correctly, since I am getting 1030 errors when I run "npm run lint", and most, if not all of them, have nothing to do with the changes I made. Is this something commonly run into when contributors are testing their code? I feel very silly not getting this right, especially for some relatively small changes to the codebase 😓

[levibuzolic]

Should sort out the sketchy null check.

Suggested change

	: this.props.overlayColor
	: this.props.overlayColor != null

[github-actions]

This PR is stale because it has been open 180 days with no activity. Remove stale label or comment or this will be closed in 7 days.

[github-actions]

This PR was closed because it has been stalled for 7 days with no activity.

[alexgshaw]

Is this not happening anymore? Would love this feature.