Skip to content

Conversation

@mattmunich
Copy link
Contributor

Fixes #1197
(and likely #745 as well)

@dnfclas
Copy link

dnfclas commented Jun 1, 2017

This seems like a small (but important) contribution, so no Contribution License Agreement is required at this point. We will now review your pull request.
Thanks,
.NET Foundation Pull Request Bot

@skendrot
Copy link
Contributor

skendrot commented Jun 1, 2017

This would mean that people using MapDetails would need to check for null now when previously they did not.
Looks like this would not change what happens when SelectedItem goes from something to nothing.
Only change is when setting via OnApplyTemplate

_detailsPresenter.Content = MapDetails == null
? SelectedItem
: MapDetails(SelectedItem);
_detailsPresenter.Content = MapDetails == null ? SelectedItem : SelectedItem != null ? MapDetails(SelectedItem) : null;
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a lot happening on one line. Could you please change to:

_detailsPresenter.Content = MapDetails == null 
    ? SelectedItem 
    : SelectedItem != null ? MapDetails(SelectedItem) : null;

_detailsPresenter.Content = MapDetails == null
? SelectedItem
: MapDetails(SelectedItem);
_detailsPresenter.Content = MapDetails == null
Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry to seem like a jerk, but please make sure the file is formatted properly. These new changes have only three spaces for a tab rather than four.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No worries, I have different standards installed on this machine - Making sure spacing is consistent IS a battle worth fighting.

: SelectedItem != null ? MapDetails(SelectedItem) : null;
}
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

One last spacing issue

@nmetulev nmetulev merged commit b33442b into CommunityToolkit:dev Jun 2, 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.

5 participants