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

Add UIBlock Interface #1050

Merged
merged 11 commits into from Mar 9, 2017
Merged

Add UIBlock Interface #1050

merged 11 commits into from Mar 9, 2017

Conversation

ryanlntn
Copy link
Contributor

@ryanlntn ryanlntn commented Mar 6, 2017

Provides a hook for third party libraries to interact with the NativeViewHierarchyManager.
Android equivalent here: https://github.com/facebook/react-native/blob/9ee815f6b52e0c2417c04e5a05e1e31df26daed2/ReactAndroid/src/main/java/com/facebook/react/uimanager/UIBlock.java

Exposes ResolveView and ResolveViewManager as public methods.

/// Executes the block.
/// </summary>
/// <param name="nativeViewHierarchyManager"></param>
void Execute(NativeViewHierarchyManager nativeViewHierarchyManager);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Fill in.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@rozele Not sure what you mean here. Do you want this method to have a default body or something?

@@ -241,4 +241,4 @@ private void ApplyArguments(string[] arguments)
}
}
}
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think in general we've been leaving newlines at the end of files, but I'm sure that's not actually consistent.

@@ -587,7 +587,7 @@ private DependencyObject ResolveView(int tag)
return view;
}

private IViewManager ResolveViewManager(int tag)
public IViewManager ResolveViewManager(int tag)
Copy link
Collaborator

@rozele rozele Mar 6, 2017

Choose a reason for hiding this comment

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

public [](start = 8, length = 6)

Hmm, I wonder if we should go one step further on Android and create a simple interface that exposes only the methods that we want exposed (e.g., ResolveView and ResolveViewManager). Thoughts @matthargett? #Closed

Copy link
Contributor

@matthargett matthargett Mar 6, 2017

Choose a reason for hiding this comment

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

maybe. I'd need to look at the consumption of the methods being exposed, but it's not jumping out at me in this diff. Also, since its only two methods and they take a POD and return a pure interface, there isn't a coupling problem -- just an ISP problem. #Closed

Copy link
Contributor Author

@ryanlntn ryanlntn Mar 7, 2017

Choose a reason for hiding this comment

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

While I think that makes sense from a design standpoint I think there is probably more value to having a more similar interfaces between iOS, Android, and Windows as it makes adding Windows support to existing extensions easier. #Closed

/// view logic after all currently queued view updates have completed.
/// </summary>
/// <param name="block"></param>
public void AddUIBlock(IUIBlock block)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Empty comment.

/// view logic after all currently queued view updates have completed.
/// </summary>
/// <param name="block"></param>
public void AddUIBlock(IUIBlock block)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Empty comment

@@ -575,7 +575,7 @@ public void ShowPopupMenu(int tag, string[] items, ICallback success)
throw new NotImplementedException();
}

private DependencyObject ResolveView(int tag)
public DependencyObject ResolveView(int tag)
{
Copy link
Collaborator

Choose a reason for hiding this comment

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

Missing comments on public classes produce warnings.

@@ -587,7 +587,7 @@ private DependencyObject ResolveView(int tag)
return view;
}

private IViewManager ResolveViewManager(int tag)
public IViewManager ResolveViewManager(int tag)
{
Copy link
Collaborator

Choose a reason for hiding this comment

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

Missing comments on public classes produce warnings.

/// Enqueues a operation to execute a UIBlock.
/// </summary>
/// <param name="block"></param>
public void EnqueueUIBlock(IUIBlock block)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Missing comment.

@rozele
Copy link
Collaborator

rozele commented Mar 9, 2017

🕐

/// Enqueues UIBlock to be executed.
/// </summary>
/// <param name="block"></param>
public void AddUIBlock(IUIBlock block)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Missing comment.

@ryanlntn
Copy link
Contributor Author

ryanlntn commented Mar 9, 2017

@rozele Updated the comments.

@rozele
Copy link
Collaborator

rozele commented Mar 9, 2017

Closing and reopening to trigger CLA bot

@rozele rozele closed this Mar 9, 2017
@rozele rozele reopened this Mar 9, 2017
@msftclas
Copy link

msftclas commented Mar 9, 2017

@ryanlntn,
Thanks for having already signed the Contribution License Agreement. Your agreement was validated by Microsoft. We will now review your pull request.
Thanks,
Microsoft Pull Request Bot

@matthargett matthargett merged commit 5fd6e27 into microsoft:master Mar 9, 2017
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.

6 participants