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

Adding OrbitView control #1160

Merged
merged 15 commits into from
Jun 27, 2017
Merged

Adding OrbitView control #1160

merged 15 commits into from
Jun 27, 2017

Conversation

nmetulev
Copy link
Contributor

@nmetulev nmetulev commented May 12, 2017

From love at Build 2017

Issue: #1154

@skendrot
Copy link
Contributor

skendrot commented May 12, 2017

Could this be accomplished by restyling the ListView? https://web.archive.org/web/20080214170102/http://www.beacosta.com/blog/?p=40

@nmetulev
Copy link
Contributor Author

@skendrot, I'm sure it can. Any reason why it should?

@mdtauk
Copy link

mdtauk commented May 15, 2017

Perhaps a more generic name like OrbitView, or NetworkView.

Also it would be great to have Pinch to Zoom support, where elements maintain their size, but the spacing between items increases or decreases.

Support for animations if an item is selected programmatically, so you can animate zooming close to an item, or away from it. The view changes to show multiple items if multi-select is enabled.

An option to display a flyout beside the selected object, instead of having to have a static detail view to the side.

@nmetulev
Copy link
Contributor Author

Thanks for the feedback @mdtauk. I think all those suggestions are great, and I can see them as potential features for a v2. We should focus on having a solid simple control for initial release.

On the name, OrbitView is a great suggestion. Let me see if there is a way to do polling for the names

@nmetulev
Copy link
Contributor Author

Trying something new.

Name poll (add your reaction to vote for the appropriate item):

  • SpaceView (thumbs up)
  • GraphView (thumb down)
  • OrbitView (laugh)
  • SateliteView (hooray)

@nmetulev
Copy link
Contributor Author

nmetulev commented Jun 2, 2017

Looks like people like OrbitView the most. Unless there is no uproar in the next few days, I'll change the name to OrbitVIew :)

@nmetulev nmetulev changed the title Adding SpaceView control Adding OrbitView control Jun 20, 2017
@nmetulev nmetulev added this to the v1.5 milestone Jun 20, 2017
Copy link
Contributor

@Odonno Odonno left a comment

Choose a reason for hiding this comment

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

Seems good.

var control = element as OrbitViewItemControl;
var orbitViewItem = item as OrbitViewItem;
FrameworkElement orbitViewElement = null;

Copy link
Contributor

Choose a reason for hiding this comment

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

can we declare of possible types here using as so we don't do is followed by as ?

Copy link
Member

Choose a reason for hiding this comment

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

I'm a big fan of is because it is more descriptive, as should be cast.


using Microsoft.Toolkit.Uwp.UI.Controls;
using System;
using System.Collections.Generic;
Copy link
Member

Choose a reason for hiding this comment

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

There are a lot of unused references, please remove it

/// </summary>
public sealed partial class OrbitViewPage : Page
{
ObservableCollection<DeviceItem> Devices = new ObservableCollection<DeviceItem>();
Copy link
Member

Choose a reason for hiding this comment

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

Devices and random variables aren't following the toolkit naming convention

Devices.Add(new DeviceItem() { Distance = random.Next(1, 10) / 10f, Label = "My Phone", Symbol = Symbol.CellPhone });
}

private void itemInvoked(object sender, OrbitViewItemClickedEventArgs e)
Copy link
Member

Choose a reason for hiding this comment

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

Event name should start with a capital letter

@@ -0,0 +1,90 @@
<Page
Copy link
Member

Choose a reason for hiding this comment

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

Please use Xaml styler extension in any xaml file. (once you save any xaml file it will apply our styles)

using Windows.UI.Xaml.Media;
using Windows.UI.Xaml.Navigation;

// The Blank Page item template is documented at https://go.microsoft.com/fwlink/?LinkId=234238
Copy link
Member

Choose a reason for hiding this comment

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

Please remove this comment

if (_panel == null && ItemsPanelRoot != null)
{
_panel = ItemsPanelRoot as OrbitViewPanel;
_panel.ItemArranged += OrbitViewPanel_ItemArranged;
Copy link
Member

Choose a reason for hiding this comment

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

Should we unsubscribe from this events before subscribe as it seems to me that this function is will repeat.

var control = element as OrbitViewItemControl;
var orbitViewItem = item as OrbitViewItem;
FrameworkElement orbitViewElement = null;

Copy link
Member

Choose a reason for hiding this comment

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

I'm a big fan of is because it is more descriptive, as should be cast.

}

/// <summary>
/// Identifies the <see cref="DistanceProperty"/> property
Copy link
Member

Choose a reason for hiding this comment

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

Documentation here should refer to Distance not DistanceProperty, please apply this comment on this file

/// Identifies the <see cref="DistanceProperty"/> property
/// </summary>
public static readonly DependencyProperty DistanceProperty =
DependencyProperty.Register("Distance", typeof(double), typeof(OrbitViewItem), new PropertyMetadata(0.5));
Copy link
Member

Choose a reason for hiding this comment

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

Also please use nameof(Distance) instead of "Distance" also please apply to the rest of the file :)

/// <summary>
/// Identifies the <see cref="IsClickEnabledProperty"/> property
/// </summary>
public static readonly DependencyProperty IsClickEnabledProperty =
Copy link
Member

Choose a reason for hiding this comment

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

Same as other property the documentation is refereing to wrong property and please use nameof

@IbraheemOsama IbraheemOsama merged commit d588c74 into CommunityToolkit:dev Jun 27, 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.

7 participants