Expander #478

VladislavAntonyuk · 2022-07-01T00:35:03Z

Description of Change

New Expander control.
Uses custom implementation on Android and iOS and native WinUI expander control.

Linked Issues

Fixes [Proposal] Expander #109

PR Checklist

Has a linked Issue, and the Issue has been approved(bug) or Championed (feature/proposal)
Has tests (if omitted, state reason in description)
Has samples (if omitted, state reason in description)
Rebased on top of main at time of PR
Changes adhere to coding standard
Documentation created or updated: https://github.com/MicrosoftDocs/CommunityToolkit/pull/XYZ

Additional information

pictos

Great work man, I did a quick review. I'll re-review again tomorrow since I'm a little sleepy and I may have missed something.

samples/CommunityToolkit.Maui.Sample/ViewModels/Views/ExpanderViewModel.cs

src/CommunityToolkit.Maui.Core/Interfaces/IExpander.cs

src/CommunityToolkit.Maui.Core/Views/Expander/PlatformView/MauiExpanderExtensions.shared.cs

pictos

@VladislavAntonyuk Thanks for this PR. I have some code suggestions and also found some bugs in all platforms. I'll do the Android and Windows in another post since the assets are on my Windows machine.

General

IMHO, since Windows provide control for that we should try to follow its guidelines, for example on Windows the header has an arrow icon and the controls have a better arrangement, we should mimic that for Android and Apple as well.

iOS / macOS

We need to click two times to collapse or expand the control, as you can see in the video below.

Uploading Screen Recording 2022-07-05 at 20.13.22.mov…

The Baboon expander doesn't appear

The simple and multiple expanders doesn't work as well, they aren't renderer on screen. And the size isn't correctly applied to the ListView cell.

pictos · 2022-07-05T21:15:57Z

src/CommunityToolkit.Maui.Core/Interfaces/IExpander.shared.cs

+	public ExpandDirection Direction { get; }
+
+	/// <summary>
+	/// Event occurred when IsExpanded changed.


This is not an event, can we change the term? Maybe method? Let's remember that we shouldn't influence the implementor in any way

pictos · 2022-07-05T21:32:26Z

src/CommunityToolkit.Maui.Core/Views/Expander/PlatformView/MauiExpander.android.cs

+		}
+
+		Header.Clickable = true;
+		Header.SetOnClickListener(new HeaderClickEventListener(this));


So this one is tricky. Because if we use the Listener and the dev wants to customize it then he will lose all our implementation and will have to copy and paste our HeaderClickEventListener, since it's internal, is it what we want?

On the other hand, we can use the Click event to implement our logic, that way the dev can add his on delegate and keep our implementation as well.

pictos · 2022-07-05T21:38:02Z

src/CommunityToolkit.Maui.Core/Views/Expander/PlatformView/MauiExpander.android.cs

+
+	partial void Draw()
+	{
+		if (Header is null || Content is null)


I see that we need to set the Header and the Content in order to use the Expander, but this is not very clear. Should add a log message/throw an exception telling the dev that he should provide both views?

I would say that could be a good idea to provide a default Header, maybe a BoxView? (If that isn't too hard)

pictos · 2022-07-05T21:41:38Z

src/CommunityToolkit.Maui.Core/Views/Expander/PlatformView/MauiExpander.macios.cs

+
+	partial void Draw()
+	{
+		if (Header is null || Content is null)


I see that we need to set the Header and the Content in order to use the Expander, but this is not very clear. Should add a log message/throw an exception telling the dev that he should provide both views?

I would say that could be a good idea to provide a default Header, maybe a BoxView? (If that isn't too hard)

pictos · 2022-07-05T21:42:42Z

src/CommunityToolkit.Maui.Core/Views/Expander/PlatformView/MauiExpander.macios.cs

+		}
+
+		var expanderGesture = new UITapGestureRecognizer();
+		expanderGesture.AddTarget(() => {


Just a small suggestion (:

Suggested change

expanderGesture.AddTarget(() => {

expanderGesture.AddTarget(() =>

{

pictos · 2022-07-05T21:59:59Z

src/CommunityToolkit.Maui.Core/Handlers/Expander/ExpanderHandler.shared.cs

+#if WINDOWS
+		VirtualView.IsExpanded = false;
+		VirtualView.ExpandedChanged(false);
+#else
+		VirtualView.IsExpanded = !e.IsCollapsed;
+		VirtualView.ExpandedChanged(!e.IsCollapsed);
+#endif


Can we merge those? They have the same names for properties and method. And !e.IsCollapsed should be false for Windows, or not?

pictos · 2022-07-05T22:05:53Z

src/CommunityToolkit.Maui.Core/Views/Expander/PlatformView/MauiExpander.shared.cs

+	/// <summary>
+	/// Event invoked when IsExpanded changed
+	/// </summary>
+	public event EventHandler<ExpanderCollapsedEventArgs> Collapsed


We shouldn't use event handlers here. Core shouldn't force any pattern. The best solution is to call the IExpander.ExpandedChanged method here.

If we've any Event Handler exposed (public) in another control we should fix it.

pictos · 2022-07-05T22:07:28Z

src/CommunityToolkit.Maui.Core/Views/Expander/PlatformView/MauiExpanderExtensions.shared.cs

+/// <summary>
+/// Extension methods to support <see cref="IExpander"/>
+/// </summary>
+public static partial class MauiExpanderExtensions


We don't need the partial here

Suggested change

public static partial class MauiExpanderExtensions

public static class MauiExpanderExtensions

pictos · 2022-07-05T22:08:07Z

src/CommunityToolkit.Maui.Core/Views/Expander/PlatformView/MauiExpanderExtensions.shared.cs

+	/// </summary>
+	public static void SetIsExpanded(this MauiExpander mauiExpander, bool isExpanded)
+	{
+		if (mauiExpander.IsExpanded != isExpanded)


Do we need this check? This check should be in the IsExpanded.Setter, that way will check for any type of value change.

pictos · 2022-07-06T00:07:49Z

Windows

On Windows, when I resize the App window the Expander stop to works on CollectionView, as you can see in the picture below Bradon's card should be expanded (arrow down) but isn't. I got the notification and everything, but the card doesn't expand.

On the Baboons page, there's odd behavior, when I expand it changes its position and horizontal size, not sure if that's automatically calculated, but that could break some layouts out there. See the video below.

expander.mp4

Android

On the Baboon page the content doesn't respect the screen size

Is hard to trigger the expander on mobile looks like there's a delay between the time that I pressed the header and the animation to collapse/expand starts.
Didn't renderer in ListView and CollectionView

VladislavAntonyuk · 2022-07-09T02:46:04Z

Thank you for detailed review @pictos.
I will try to fix all layout issues.
As for iOS, this may be related to what we discussed: dotnet/maui#3643

Windows uses native WinUI control, I didn't create custom MauiExpander.Windows. So it's a default behavior. There is a property to use all available horizontal space. I can set it to true by default

pictos · 2022-07-09T02:50:35Z

@VladislavAntonyuk for ListView maybe you can try this. I believe that the dev will need to perform some properties on the ListView but you can call the Cell.ForceUpdateSize method.

VladislavAntonyuk · 2022-07-09T02:53:45Z

@VladislavAntonyuk for ListView maybe you can try this. I believe that the dev will need to perform some properties on the ListView but you can call the Cell.ForceUpdateSize method.

Is it for sample? I believe it is not correct to call this method inside Expander. It's not its responsibility

pictos · 2022-07-09T04:21:59Z

@VladislavAntonyuk for ListView maybe you can try this. I believe that the dev will need to perform some properties on the ListView but you can call the Cell.ForceUpdateSize method.

Is it for sample? I believe it is not correct to call this method inside Expander. It's not its responsibility

I thought to call that inside the expander, if the parent is a ListView. So the expander will communicate to the ListView that it needs to resize the cell.

src/CommunityToolkit.Maui.Core/Views/Expander/PlatformView/MauiExpanderExtensions.shared.cs

Add samples Fix BindingContext is null Android Expander iOS Configure Header Do not arrange content on expanded change More samples, fix Android InvalidateMeasure, add animation Animation Android. fix WIndows build Update sample Fix PR comments Fix Apple animation Fix pipeline Expander tests Summary

TheCodeTraveler · 2022-07-15T16:58:10Z

Just adding the do-not-merge and pending documentation tags so that we don't forget to update the docs!

VladislavAntonyuk · 2022-07-17T17:12:49Z

@pictos unfortunately Cell.ForceUpdateSize doesn't work.
Android:
I cannot reproduce the issue with rendering inside ListView/CollectionView:

Fixed layout in C# Sample:

Windows:
CollectionView incorrectly calculates the size of the cell:

and

As you can see ListView correctly displays content. On CollectionView the first cell renderers correctly. The second cell is opened, but there is no content.

VladislavAntonyuk · 2022-07-17T18:45:58Z

@pictos I've just updated the sample and copied/pasted the code from XCT expander to compare the behavior.
On Android I see the same results for both implementations.
On Windows, the main question here, is which one do we want to use?

TheCodeTraveler · 2022-08-04T20:30:07Z

/dotnet format

VladislavAntonyuk · 2022-08-04T20:47:25Z

/dotnet format

hope it won't delete the dotnet world

jmbowman1107 · 2022-10-26T14:09:17Z

When will this be merged?

TheCodeTraveler · 2022-11-06T20:25:37Z

Hey Vlad! I made a couple tweaks to Expander to simplify how it works and to reduce memory usage.

In Constructor, initialize Grid with 2 RowDefinitions:

public Expander()
{
  // ...
  base.Content = new Grid
  {
      RowDefinitions =
      {
  	    new RowDefinition(GridLength.Auto),
  	    new RowDefinition(GridLength.Auto)
      }
  };
}

Bind Content.IsVisible to IsExpanded
- Ensures Expander's Content is visible when expanded
- Ensures Expander's Content is not visible when not expanded
```
Content.SetBinding(IsVisibleProperty, new Binding(nameof(Expander.IsExpanded), source: this));
```

Set Rows When Direction changes:

switch (expandDirection)
{
    case ExpandDirection.Down:
  	  ContentGrid.SetRow(Header, 0);
  	  ContentGrid.SetRow(Content, 1);
  	  break;

    case ExpandDirection.Up:
  	  ContentGrid.SetRow(Header, 1);
  	  ContentGrid.SetRow(Content, 0);
  	  break;

    default:
  	  throw new NotSupportedException($"{nameof(ExpandDirection)} {expandDirection} is not yet supported");
}

TheCodeTraveler

Hey Vlad!

ListView support isn't working properly on iOS.

I confirmed that it is working on Android.

Simulator.Screen.Recording.-.iPhone.14.-.2022-11-06.at.12.27.25.mp4

TheCodeTraveler · 2022-11-08T00:59:35Z

@VladislavAntonyuk Since ListView is (basically) deprecated in favor of CollectionView, let's move forward with the following tasks so that we can merge this PR to ensure we get Expander into v1.4.0:

Open Bug Issue noting that Expander does not support ListView
Remove ListView support from Expander
- In ForceUpdateLayoutSizeForItemsView(), replace code in the if(element is ListView listView) block with throw new NotSupportedException($"{nameof(Expander)} is not yet supported on ListView");
Open Docs PR for Expander
- Add warning about not supporting ListView

Once we've finished these, I'm happy to Approve + Merge this PR 🙌

TheCodeTraveler · 2022-11-08T01:00:26Z

src/CommunityToolkit.Maui/Views/Expander/Expander.shared.cs

+			if (element is ListView listView)
+			{
+				foreach (var child in listView.AllChildren.OfType<Cell>())
+				{
+					child.ForceUpdateSize();
+				}


Here's my recommendation to remove ListView support from this release of Expander

Suggested change

if (element is ListView listView)

{

foreach (var child in listView.AllChildren.OfType<Cell>())

{

child.ForceUpdateSize();

}

if (element is ListView listView)

{

throw new NotSupportedException($"{nameof(Expander)} is not yet supported inside {nameof(ListView)}");

I suggest keeping it, because ListView works much better on WinUI.

VladislavAntonyuk requested review from TheCodeTraveler and pictos July 1, 2022 00:35

pictos reviewed Jul 1, 2022

View reviewed changes

VladislavAntonyuk force-pushed the Expander branch from 5a4669d to c49abaf Compare July 5, 2022 15:58

VladislavAntonyuk requested a review from bijington July 5, 2022 20:58

VladislavAntonyuk marked this pull request as ready for review July 5, 2022 21:02

pictos requested changes Jul 5, 2022

View reviewed changes

veikkoeeva reviewed Jul 10, 2022

View reviewed changes

src/CommunityToolkit.Maui.Core/Views/Expander/PlatformView/MauiExpanderExtensions.shared.cs Outdated Show resolved Hide resolved

VladislavAntonyuk added 2 commits July 15, 2022 15:20

Add Command and CommandParameter

59ebeab

VladislavAntonyuk force-pushed the Expander branch from 6261033 to 59ebeab Compare July 15, 2022 12:20

TheCodeTraveler added pending documentation This feature requires documentation do not merge Do not merge this PR labels Jul 15, 2022

Merge branch 'main' into Expander

8da32c7

VladislavAntonyuk added 3 commits July 17, 2022 20:14

Detect cell and force update size

e5e8420

Merge remote-tracking branch 'origin/Expander' into Expander

ffca17a

Expander2

a32c2af

VladislavAntonyuk and others added 2 commits August 3, 2022 22:26

Merge branch 'main' into Expander

cd41764

Register Expander page

c4085c5

Merge branch 'main' into Expander

0b9a9a0

TheCodeTraveler and others added 4 commits October 12, 2022 15:07

Merge branch 'main' into Expander

600ec4a

Merge branch 'main' into Expander

f1729c2

Fix formatting

8b8917c

Merge branch 'main' into Expander

3c6fe52

TheCodeTraveler added the hacktoberfest-accepted A PR that has been approved during Hacktoberfest label Nov 1, 2022

TheCodeTraveler added 9 commits October 31, 2022 19:36

Merge branch 'main' into Expander

5e5fb12

Remove incorrect namespace

e8f5d23

Update Sample, Use Static Methods

422aaf5

Update ExpanderPageCS.cs

29acda6

Ensure Command Fires

79668f3

Remove Children Before Creating New Grid

4696d39

Add InvalidEnumArgumentException Test

9c882bb

Avoid Re-initializing Grid

38096ff

Merge branch 'main' into Expander

5205600

TheCodeTraveler requested changes Nov 6, 2022

View reviewed changes

TheCodeTraveler removed the do not merge Do not merge this PR label Nov 7, 2022

TheCodeTraveler added this to the v1.4.0 milestone Nov 7, 2022

TheCodeTraveler reviewed Nov 8, 2022

View reviewed changes

VladislavAntonyuk and others added 6 commits November 8, 2022 09:59

Merge branch 'main' into Expander

51400bc

Merge branch 'main' into Expander

6f35264

Fixes Expander crash when Header or Content is null

41b3bb6

Merge branch 'main' into Expander

cd17f0b

Add Title

8002dae

Disable ListView + CollectionView support

5f719c7

TheCodeTraveler merged commit 1ea488c into main Nov 8, 2022

TheCodeTraveler deleted the Expander branch November 8, 2022 21:54

github-actions bot locked and limited conversation to collaborators Nov 22, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Expander #478

Expander #478

VladislavAntonyuk commented Jul 1, 2022 •

edited

Loading

pictos left a comment

pictos left a comment

pictos Jul 5, 2022

pictos Jul 5, 2022

pictos Jul 5, 2022

pictos Jul 5, 2022

pictos Jul 5, 2022

pictos Jul 5, 2022

pictos Jul 5, 2022

pictos Jul 5, 2022

pictos Jul 5, 2022

pictos commented Jul 6, 2022

VladislavAntonyuk commented Jul 9, 2022 •

edited

Loading

pictos commented Jul 9, 2022

VladislavAntonyuk commented Jul 9, 2022

pictos commented Jul 9, 2022

TheCodeTraveler commented Jul 15, 2022

VladislavAntonyuk commented Jul 17, 2022

VladislavAntonyuk commented Jul 17, 2022

TheCodeTraveler commented Aug 4, 2022

VladislavAntonyuk commented Aug 4, 2022

jmbowman1107 commented Oct 26, 2022

TheCodeTraveler commented Nov 6, 2022

TheCodeTraveler left a comment

TheCodeTraveler commented Nov 8, 2022 •

edited

Loading

TheCodeTraveler Nov 8, 2022 •

edited

Loading

VladislavAntonyuk Nov 8, 2022

	expanderGesture.AddTarget(() => {
	expanderGesture.AddTarget(() =>
	{

	public static partial class MauiExpanderExtensions
	public static class MauiExpanderExtensions

Expander #478

Expander #478

Conversation

VladislavAntonyuk commented Jul 1, 2022 • edited Loading

Description of Change

Linked Issues

PR Checklist

Additional information

pictos left a comment

Choose a reason for hiding this comment

pictos left a comment

Choose a reason for hiding this comment

General

iOS / macOS

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

pictos commented Jul 6, 2022

Windows

Android

VladislavAntonyuk commented Jul 9, 2022 • edited Loading

pictos commented Jul 9, 2022

VladislavAntonyuk commented Jul 9, 2022

pictos commented Jul 9, 2022

TheCodeTraveler commented Jul 15, 2022

VladislavAntonyuk commented Jul 17, 2022

VladislavAntonyuk commented Jul 17, 2022

TheCodeTraveler commented Aug 4, 2022

VladislavAntonyuk commented Aug 4, 2022

jmbowman1107 commented Oct 26, 2022

TheCodeTraveler commented Nov 6, 2022

TheCodeTraveler left a comment

Choose a reason for hiding this comment

TheCodeTraveler commented Nov 8, 2022 • edited Loading

TheCodeTraveler Nov 8, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

VladislavAntonyuk commented Jul 1, 2022 •

edited

Loading

VladislavAntonyuk commented Jul 9, 2022 •

edited

Loading

TheCodeTraveler commented Nov 8, 2022 •

edited

Loading

TheCodeTraveler Nov 8, 2022 •

edited

Loading