Skip to content
This repository was archived by the owner on Dec 5, 2024. It is now read-only.

Conversation

@StanleyGoldman
Copy link
Contributor

@StanleyGoldman StanleyGoldman commented Sep 2, 2017

Part of #251
#261 depends on this

Depends on:

…ser-settings-view

# Conflicts:
#	src/UnityExtension/Assets/Editor/GitHub.Unity/UI/SettingsView.cs
@StanleyGoldman StanleyGoldman force-pushed the enhancements/user-settings-view branch from 1c3ad0d to 89569bc Compare September 5, 2017 15:27
@StanleyGoldman StanleyGoldman force-pushed the enhancements/user-settings-view branch from ec7718e to 344beec Compare September 6, 2017 22:54
Copy link
Member

@shana shana left a comment

Choose a reason for hiding this comment

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

Yup this looks good! Got some minor nitpicks and a question that might require some code change.


public override bool IsBusy
{
get { return ActiveView.IsBusy; }
Copy link
Member

Choose a reason for hiding this comment

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

This change is probably a glitchy merge, the property should be at the bottom

public override void OnDataUpdate()
{
base.OnDataUpdate();
if (userSettingsView != null)
Copy link
Member

Choose a reason for hiding this comment

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

When is this ever null?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are right, removing the null check


public override bool IsBusy
{
get { return isBusy || userSettingsView.IsBusy; }
Copy link
Member

Choose a reason for hiding this comment

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

Bottom of file

@StanleyGoldman StanleyGoldman merged commit 6487aad into master Sep 18, 2017
@StanleyGoldman StanleyGoldman deleted the enhancements/user-settings-view branch September 18, 2017 19:07
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants