Addition of MaxOpenBlades property on BladeView control #1049

Depechie · 2017-03-24T19:23:12Z

Cfr #627 for the discussion and a demo movie posted to YouTube.

This is my vision of how we can keep screen real estate to a minimal when using blades.
The first original Azure portal had a similar feature ( although no longer present ).

In short, once we reached MaxOpenBlades count threshold of open blades with a title bar, we collapse all blades but the last one each time we add a new blade.
The sample page has been updated.

skendrot · 2017-04-04T19:01:04Z

Things I'd like to see in the PR:

Remove the BladeItemMode property (unless someone can give a concrete use case for needing the property)
Control the visual state of the BladeItem via a VisualState instead of BladeItemMode property
Change the name of MaxOpenBlades. This name says "You cannot have more than this number of blades". How about AutoCollapseCountThreshold. The default value for the property would be int.MaxValue
Allow the BladeItem to be collapsed and expanded regardless of the threshold value

Depechie · 2017-04-15T10:35:42Z

Talked with @ScottIsAFool on possibility to add VisualStates in favour of the BladeItemMode, but due to the fact that we are working with a Templated control, I can't change a property of the Parent from within the Child. So we need to stick with the coded version.

Property name is changed to AutoCollapseCountThreshold and default value is now Int.MaxValue.
Now the enlarge button is always visible ( to shrink / enlarge the blade ).
When in small mode the content of the blade is hidden, will reshow when enlarged.
Sample page changed to show the AutoCollapseCountThreshold being set to 4 for the sample.

skendrot · 2017-05-18T18:06:59Z

@Depechie Why are you saying it cannot be done? If the threshold is reached, call Collapse() on the items, in the Collapse method it fires an event that it has collapsed and sets a visual state. The BladeItem would also have an Expand method and an Expanded event. Or it could work like the Expander control and have an IsExpanded property. It could even inherit from Expander

Depechie · 2017-05-19T11:15:17Z

@skendrot I tried that... if I introduced a visual state in the style for the BladeItem, I was not able to adjust it's own width. If you look at the expander, it's changing a property of a child element. That is indeed possible...

Could be I'm missing something, so be my guest and maybe you can retry it, just fork my repo?

Do note looked at it with @ScottIsAFool so guessing we both miss something you are seeing :D

nmetulev · 2017-05-28T22:35:59Z

ping, do you guys think we can have this for 1.5?

Depechie · 2017-05-29T06:28:32Z

@nmetulev for me there aren't any open issues with it anymore... not sure what the standpoint is from the core team?
Anyhow - if you accept the PR other people can still perform additions on it if needed. Or does a PR mean each final say has to be done by the PR submitter?

ScottIsAFool

Minor code style adjustments, as long as they get done, consider this approved.

ScottIsAFool · 2017-05-30T13:53:23Z

Microsoft.Toolkit.Uwp.UI.Controls/BladeView/BladeItem.cs

+
+            _enlargeButton.Click -= EnlargeButton_Click;
+            _enlargeButton.Click += EnlargeButton_Click;
+


Remove empty line

ScottIsAFool · 2017-05-30T13:53:40Z

Microsoft.Toolkit.Uwp.UI.Controls/BladeView/BladeItem.cs

+        private void OnSizeChanged(object sender, SizeChangedEventArgs sizeChangedEventArgs)
+        {
+            if(BladeItemMode == BladeItemMode.Normal)
+                _normalModeWidth = Width;


Should have curly braces around if statement content.

ScottIsAFool · 2017-05-30T13:54:24Z

Microsoft.Toolkit.Uwp.UI.Controls/BladeView/BladeView.cs

+                var openBlades = ActiveBlades.Where(item => item.TitleBarVisibility == Visibility.Visible).ToList();
+                if (openBlades?.Count > AutoCollapseCountThreshold)
+                {
+                    for (int i = 0; i < openBlades.Count - 1; i++)


Should have curly braces around for statement content

ScottIsAFool · 2017-05-30T13:54:43Z

Microsoft.Toolkit.Uwp.UI.Controls/BladeView/BladeView.cs

@@ -116,6 +127,9 @@ private void CycleBlades()

            if (visibility == Visibility.Visible)
            {
+                if (Items == null)
+                    return;


Curly braces please.

ScottIsAFool · 2017-05-30T13:54:49Z

Microsoft.Toolkit.Uwp.UI.Controls/BladeView/BladeView.cs

+
+            var lastBlade = ActiveBlades.LastOrDefault();
+            if (lastBlade != null && lastBlade.TitleBarVisibility == Visibility.Visible)
+                lastBlade.BladeItemMode = BladeItemMode.Normal;


Curly braces please.

ScottIsAFool · 2017-05-30T13:55:08Z

Microsoft.Toolkit.Uwp.UI.Controls/BladeView/BladeItem.cs

@@ -47,11 +54,29 @@ protected override void OnApplyTemplate()

            _closeButton.Click -= CloseButton_Click;
            _closeButton.Click += CloseButton_Click;
+
+            if (_enlargeButton == null)
+                return;


Curly braces please.

ScottIsAFool · 2017-05-30T13:55:19Z

Microsoft.Toolkit.Uwp.UI.Controls/BladeView/BladeItem.Properties.cs

+
+            ContentPresenter content = bladeItem.GetTemplateChild("ContentPresenter") as ContentPresenter;
+            if (content == null)
+                return;


Curly braces please.

skendrot · 2017-05-30T14:26:49Z

@Depechie We don't need to set the width when a blade item is collapsed. Create a Visual State for expanded and collapsed. When collapsed set the visibility of the ContentPresenter and the title bar to Collapsed. This will make the entire control the width you are expecting.
I also wouldn't expect collapsing when in full screen mode.
The button needs to change from collapse to expand depending on the visual state

skendrot · 2017-05-30T14:31:00Z

What are other thoughts about making the BladeItem an Expander control?

Depechie · 2017-05-30T14:34:05Z

@skendrot Changing it to an expander control is maybe not such a bad idea indeed
But this means throwing out current implementation I guess

About the Visual States, if you can show this can be done through my PR, I'm happy to learn :)

Add a visual state for expanded/collaped to the blade item

Control the blade item visual state when the mode changes

Depechie · 2017-06-21T07:20:24Z

@nmetulev was this q directed to me?
If so... I did all the code for the idea I myself proposed - the idea is working as expected and after the code reviews all changes were made to adjust for the remarks.
Seems this was the way other PR's worked.
Now at the end after the work, only @skendrot is suggesting to refactor the control.

So does your question mean you guys now only want the control change in there when it is a real expander instead of the current rework?
If so, count me out, would have been nice to at least think upfront and don't let community people spend time to try to get a PR that has no value in the end.
Would still guess this PR can be added and put the refactor up for future release, but long story short, just drop the PR if an expander is needed.

skendrot · 2017-06-21T15:40:47Z

@Depechie If changing to an expander is too difficult I can handle the changes

skendrot · 2017-06-21T15:41:48Z

Microsoft.Toolkit.Uwp.UI.Controls/BladeView/BladeView.cs

+            // For now we skip this feature when blade mode is set to fullscreen
+            if (AutoCollapseCountThreshold > 0 && BladeMode != BladeMode.Fullscreen && ActiveBlades.Any())
+            {
+                var openBlades = ActiveBlades.Where(item => item.TitleBarVisibility == Visibility.Visible).ToList();


Should check the expand mode instead of the visibility of the title bar. This could be set by the user

My understanding was that the TitleBarVisibility property was used to pin blades to the screen.
I want to filter the pinned once out, they are not touched by the auto resize mechanism.
@skendrot Is there another property that tells me this? I don't seem to find it...

You're right! Thanks for pointing that out

skendrot · 2017-06-21T15:42:31Z

Microsoft.Toolkit.Uwp.UI.Controls/BladeView/BladeView.cs

+            if (AutoCollapseCountThreshold > 0 && BladeMode != BladeMode.Fullscreen && ActiveBlades.Any())
+            {
+                var openBlades = ActiveBlades.Where(item => item.TitleBarVisibility == Visibility.Visible).ToList();
+                if (openBlades?.Count > AutoCollapseCountThreshold)


no need to check for null, guaranteed to be a list

skendrot · 2017-06-21T15:43:51Z

Microsoft.Toolkit.Uwp.UI.Controls/BladeView/BladeView.cs

+                {
+                    for (int i = 0; i < openBlades.Count - 1; i++)
+                    {
+                        openBlades[i].BladeItemMode = BladeItemMode.Small;


Will this close ALL open blades? Or just the amount under the threshold count?

All open blades except the last one, IF the amount of blades drawn is over the threshold.

skendrot · 2017-06-21T15:44:58Z

Microsoft.Toolkit.Uwp.UI.Controls/BladeView/BladeView.cs

@@ -133,6 +151,12 @@ private void CycleBlades()

            BladeClosed?.Invoke(this, blade);
            ActiveBlades.Remove(blade);
+
+            var lastBlade = ActiveBlades.LastOrDefault();
+            if (lastBlade != null && lastBlade.TitleBarVisibility == Visibility.Visible)


Shouldn't check the titlebar visibility. This can be set to collapsed by the user but still want the last blade to be expanded

Same remark as above, I don't want to auto size pinned blades.

nmetulev · 2017-06-22T00:03:16Z

Apologies for randomizing you :(. My vote is to keep the changes you've done as it seems that your changes are very useful and needed. We should then open a new issue to discuss further updates/changes to the control before we start a new PR.

Looks like @skendrot has some minor requests that should be looked at before merging the control. Do you think you can take a look at those this week?

Depechie · 2017-06-22T19:30:55Z

@nmetulev done the changes and commented on the other questions of @skendrot - but guessing that has to be reviewed again. Because could be my view is not the same as the UWP toolkit team.

skendrot · 2017-06-22T21:19:04Z

Tested this out. If I set the AutoCollapse to 3 and open three blades they are all open. When I open the forth it collapses the first three and then leaves the last one open. If I add a fifth it collapses the fourth and leaves the fifth open.

I was expecting it to always leave the last three open. This is probably a misunderstanding on my part but I'm wondering if others will think the same.

Should it only leave the last one open? Or should it leave the amount open that you specify?

Depechie · 2017-06-22T21:26:59Z

Yeah... every person could have a different view on this I guess!
Why I wanted this was to allow for some sort of action flow ( like the azure portal ), where a blade is open and doing something there will trigger a new blade to appear that needs the users attention.
This can continue, meaning the previous open blades are actually a distraction.
Thus to not clutter the screen and keep the user engaging with the blade that needs attention, I want all blades closed except the last one.
The threshold is there because of screen size differences and blade widths.

We could add an extra property to also accomplish the other need, to start collapsing blades After the threshold count AND keep in mind the new property called KeepOpenBlades for example to not close the x last ones where x equals that keep open property.
But still, would not do this in same PR for testing and evaluation reasons.

skendrot · 2017-06-22T21:33:16Z

Yea, completely understand the usecase and I agree the open blades can be a distraction. Definitely no need to update this PR to handle a different use case.

I'm good with things as is. @nmetulev, what are your thoughts on the workflow or property name? Do they communicate the intended functionality properly?

nmetulev · 2017-06-22T22:46:37Z

Reading your description again, it makes sense why you are doing it this way. But just from the property name, I had the same assumption as @skendrot, that it would only keep up to max open blades. How do you feel about changing the name to something else, maybe along the lines of "CollapseAllButLastThreshold"

skendrot · 2017-06-22T22:48:23Z

The original property was named MaxOpenBlades. The current name is AutoCollapseCountThreshold

nmetulev · 2017-06-22T22:49:20Z

Oh, that's much better, apologies. I'm good with that name :)

nmetulev · 2017-06-22T22:50:21Z

We should make sure we add it to the documentation though before we merge it :)

nmetulev · 2017-06-25T20:40:58Z

@Depechie, could you add the new property to the documentation before we merge?

Depechie · 2017-06-26T07:18:59Z

@nmetulev ok will try to fit that in this week!

nmetulev · 2017-06-27T03:09:12Z

Thanks @Depechie, let me know if you can make it by Wednesday, or if you need help. Code freeze for 1.5 is Wednesday EOD PST.

Added new AutoCollapseCountThreshold to documents

Depechie · 2017-06-27T19:39:50Z

@nmetulev could some people double check for English spelling error, and is the wording ok? Updated the BladeView.md.

skendrot · 2017-06-27T19:40:19Z

docs/controls/BladeView.md

@@ -66,6 +66,12 @@ Here is an example of a BladeView where the `BladeMode` property is binded to a

 ```

+## AutoCollapseCountThreshold
+
+If you want to use the BladeView for handling a flow of actions, you can use the `AutoCollapseCountThreshold` property to tell it to start auto collapsing BladeItems after a certain threshold count has been reached. This will also help keeping a clean, uncluttered screen real estate.


Change

This will also help keeping a clean, uncluttered screen real estate.

To

This will also help keep a clean, uncluttered screen real estate.

(change keeping to keep)

English is hard

skendrot · 2017-06-27T19:42:37Z

docs/controls/BladeView.md

+
+If you want to use the BladeView for handling a flow of actions, you can use the `AutoCollapseCountThreshold` property to tell it to start auto collapsing BladeItems after a certain threshold count has been reached. This will also help keeping a clean, uncluttered screen real estate.
+
+Let's explain this with an example; if you put the `AutoCollapseCountThreshold` property to 3, the BladeView will start counting all BladeItems that are open in the BladeView and have their `TitleBarVisibility` property set to Visible. When the n+1 BladeItem, in our case the 4th one, is being added, the BladeView will auto collaps all n BladeItems except for the last one. All additional BladeItems that are added afterwards will trigger the same effect; collapse all BladeItems except for the last open one.


Change

Let's explain this with an example

To

For example

skendrot · 2017-06-27T19:43:20Z

docs/controls/BladeView.md

+
+If you want to use the BladeView for handling a flow of actions, you can use the `AutoCollapseCountThreshold` property to tell it to start auto collapsing BladeItems after a certain threshold count has been reached. This will also help keeping a clean, uncluttered screen real estate.
+
+Let's explain this with an example; if you put the `AutoCollapseCountThreshold` property to 3, the BladeView will start counting all BladeItems that are open in the BladeView and have their `TitleBarVisibility` property set to Visible. When the n+1 BladeItem, in our case the 4th one, is being added, the BladeView will auto collaps all n BladeItems except for the last one. All additional BladeItems that are added afterwards will trigger the same effect; collapse all BladeItems except for the last open one.


Change

if you put the AutoCollapseCountThreshold property to 3

To

if you set AutoCollapseCountThreshold to 3

skendrot · 2017-06-27T19:44:18Z

docs/controls/BladeView.md

+
+If you want to use the BladeView for handling a flow of actions, you can use the `AutoCollapseCountThreshold` property to tell it to start auto collapsing BladeItems after a certain threshold count has been reached. This will also help keeping a clean, uncluttered screen real estate.
+
+Let's explain this with an example; if you put the `AutoCollapseCountThreshold` property to 3, the BladeView will start counting all BladeItems that are open in the BladeView and have their `TitleBarVisibility` property set to Visible. When the n+1 BladeItem, in our case the 4th one, is being added, the BladeView will auto collaps all n BladeItems except for the last one. All additional BladeItems that are added afterwards will trigger the same effect; collapse all BladeItems except for the last open one.


Fix spelling of collaps

the BladeView will auto collaps all

More spell checking

…ities from CommunityToolkit#1049

Depechie added 7 commits March 21, 2017 00:24

First draft of adding BladeItemMode

f97f20e

Blade enlarge

d3cba99

Resize lastblade

a3ad6f6

Code cleanup

ddba3bb

Comment

d10769a

Merge remote-tracking branch 'upstream/dev' into dev

59713c2

Rename property

5a656a3

dnfclas added the cla-required label Mar 24, 2017

Fixing headers

77893d4

deltakosh added this to the v1.5 milestone Mar 24, 2017

deltakosh requested a review from skendrot April 4, 2017 16:49

Depechie added 5 commits April 14, 2017 16:45

Merge from UpStream

81debe1

Always show change size button on BladeItem

c468100

VisualStates test

e8cb995

Hide content when in small mode

82c50a7

Adjust sample page to show AutoCollapseCountThreshold

b721fd8

ScottIsAFool approved these changes May 30, 2017

View reviewed changes

skendrot and others added 4 commits May 30, 2017 10:44

Add expanded/collapsed visual states

380993e

Add a visual state for expanded/collaped to the blade item

Control visual state

fbfb8d6

Control the blade item visual state when the mode changes

Getting code review compliant

5920cb9

Merge remote-tracking branch 'upstream/dev' into dev

722fb12

skendrot suggested changes Jun 21, 2017

View reviewed changes

Changes as requested by review

ccec769

skendrot approved these changes Jun 22, 2017

View reviewed changes

skendrot added the need documentation 📃 label Jun 22, 2017

Depechie and others added 2 commits June 27, 2017 21:35

Added new blade view animated gif for docs

b351e4d

Updated BladeView.md

8d24e6a

Added new AutoCollapseCountThreshold to documents

skendrot suggested changes Jun 27, 2017

View reviewed changes

Update BladeView.md

36a5d78

English is hard

skendrot suggested changes Jun 27, 2017

View reviewed changes

Update BladeView.md

a84abfb

More spell checking

skendrot approved these changes Jun 27, 2017

View reviewed changes

skendrot merged commit b519192 into CommunityToolkit:dev Jun 27, 2017

skendrot mentioned this pull request Jun 27, 2017

BladeView - amount of open blades #627

Closed

skendrot added a commit to skendrot/UWPCommunityToolkit that referenced this pull request Jun 27, 2017

Change BladeItem to be an Expander control with the expanding capabil…

924b3ec

…ities from CommunityToolkit#1049

skendrot mentioned this pull request Jun 27, 2017

Change BladeItem to be an Expander #1248

Merged


		_enlargeButton.Click -= EnlargeButton_Click;
		_enlargeButton.Click += EnlargeButton_Click;


		If you want to use the BladeView for handling a flow of actions, you can use the `AutoCollapseCountThreshold` property to tell it to start auto collapsing BladeItems after a certain threshold count has been reached. This will also help keeping a clean, uncluttered screen real estate.

		Let's explain this with an example; if you put the `AutoCollapseCountThreshold` property to 3, the BladeView will start counting all BladeItems that are open in the BladeView and have their `TitleBarVisibility` property set to Visible. When the n+1 BladeItem, in our case the 4th one, is being added, the BladeView will auto collaps all n BladeItems except for the last one. All additional BladeItems that are added afterwards will trigger the same effect; collapse all BladeItems except for the last open one.

Addition of MaxOpenBlades property on BladeView control #1049

Addition of MaxOpenBlades property on BladeView control #1049

Conversation

Depechie commented Mar 24, 2017

skendrot commented Apr 4, 2017 • edited Loading

Depechie commented Apr 15, 2017

skendrot commented May 18, 2017

Depechie commented May 19, 2017 • edited Loading

nmetulev commented May 28, 2017

Depechie commented May 29, 2017 • edited Loading

ScottIsAFool left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

skendrot commented May 30, 2017

skendrot commented May 30, 2017

Depechie commented May 30, 2017

Depechie commented Jun 21, 2017

skendrot commented Jun 21, 2017

skendrot Jun 21, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nmetulev commented Jun 22, 2017

Depechie commented Jun 22, 2017

skendrot commented Jun 22, 2017

Depechie commented Jun 22, 2017 • edited Loading

skendrot commented Jun 22, 2017 • edited Loading

nmetulev commented Jun 22, 2017

skendrot commented Jun 22, 2017

nmetulev commented Jun 22, 2017

nmetulev commented Jun 22, 2017

nmetulev commented Jun 25, 2017

Depechie commented Jun 26, 2017

nmetulev commented Jun 27, 2017

Depechie commented Jun 27, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

skendrot commented Apr 4, 2017 •

edited

Loading

Depechie commented May 19, 2017 •

edited

Loading

Depechie commented May 29, 2017 •

edited

Loading

skendrot Jun 21, 2017 •

edited

Loading

Depechie commented Jun 22, 2017 •

edited

Loading

skendrot commented Jun 22, 2017 •

edited

Loading