Navigation Menu

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

Extract the WebView component out of core and into an extension #1101

Merged
merged 6 commits into from Jul 14, 2019

Conversation

sbeca
Copy link
Contributor

@sbeca sbeca commented Jun 2, 2019

This is a followup PR to #1059 and #1092.

In #1092 it was decided to move RX.Network out of the core ReactXP codebase and into an extension, because it would require depending on and linking to an external dependency. #1059 made the WebView depend on an external dependency but kept it in core ReactXP so this PR moves WebView out into an extension.

NOTE: While making this move, there were two things that I didn't know how to move that would be good to get feedback on:

  1. I didn't know how to move the WebViewStyle and everything around that as the creation of styles is tied to some ReactXP internal functions. Seeing as WebViewStyle is just an extension of ViewStyle without adding anything extra, I have just removed it for now and just used ViewStyle. If this is not acceptable, please let me know how I should best include this in the extension.
  2. I don't know what RX.WebViewConstructor is used for or how best to include it in an extension. Is this needed?

@berickson1
Copy link
Collaborator

On a first pass, this looks good! Thanks for taking the time to do it. On Friday I'll spend a bit of time testing.
Re: questions above

  1. I think the move to ViewStyle is fine, since there's no differing styles between the two
  2. Looking at the code WebViewConstructor was used since WebView was defined as an interface, not an abstract class. Your changes bring the behaviour inline with other components, so I believe it's fine to remove RX.WebViewConstructor

@berickson1
Copy link
Collaborator

Validated on Android - fixed one race condition in RXPTest that was causing inaccurate test results. But LGTM

@sbeca
Copy link
Contributor Author

sbeca commented Jun 9, 2019

Review nit has been resolved

@sbeca
Copy link
Contributor Author

sbeca commented Jul 13, 2019

Conflicts with master have now been fixed

@erictraut erictraut merged commit 044d46e into microsoft:master Jul 14, 2019
@sbeca sbeca deleted the webview-extension branch July 14, 2019 23:44
berickson1 pushed a commit that referenced this pull request Sep 12, 2019
This is a follow-on to #1101 - the WebView extension is working fine and the doc exists, it just wasn't linked int
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.

None yet

3 participants