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

Add Collapse Support to ListViewGroup #3067

Closed
lonitra opened this issue Apr 16, 2020 · 5 comments · Fixed by #3155
Closed

Add Collapse Support to ListViewGroup #3067

lonitra opened this issue Apr 16, 2020 · 5 comments · Fixed by #3155
Assignees
Labels
api-approved (4) API was approved in API review, it can be implemented api-suggestion (1) Early API idea and discussion, it is NOT ready for implementation

Comments

@lonitra
Copy link
Member

lonitra commented Apr 16, 2020

Is your feature request related to a problem? Please describe.

As of now, there's not an option for a user to hide items under a ListViewGroup that they aren't interested in seeing at the time, similar to what windows like File Explorer provide. This could result in endless scrolling if there are many ListViewGroups and many items under each group. As #2623 suggested, a collapsible/collapse property would be a nice touch to give users flexibility and can especially be useful for clutter management purposes.

Describe the solution you'd like

Add the ability for users to make items in a group collapsible.

API:

public class ListViewGroup 
{
    public bool Collapsible { get; set; }
    public bool Collapsed { get; set; }
}

Example:

var lvGroup = new ListViewGroup 
{
    Collapsible = true,
    Collapsed = false
};

An excellent visualization of what this might look like from the original issue:
image

Will this feature affect UI controls?

Yes.

Will VS Designer need to support the feature?
It would be nice. I would imagine the Designer would have an area carved out in the ListViewGroup Collection Editor where Collapsible/Collapsed can be toggled. Something like so:
designerex

I also imagine that the changes would immediately visible to the user in the designer, again similar to what is shown in the original issue.

What impact will it have on accessibility?
I'm not quite sure here.

Will this feature need to be localized or be localizable?
Aside from some work relating to accessibility namely perhaps what a talkback would use to describe the property, no I do not think this needs to be localized/localizable.


Update
After implementing Collapsible/Collapsed property for ListViewGroup as done so in #3155 , we realize that this will be an issue with the design-time serializer, which orders properties assignment statement alphabetically. This means that Collapsed will come before Collapsible.
Suppose a user wanted to set the properties like so in designer:
image
The serializer will generate:

this.listViewGroup1.Collapsed = true;
this.listViewGroup1.Collapsible = true;

Since Collapsed is no-op when Collapsible = false (which is the default), the end result will be Collapsed = false and Collapsible = true , not what the user intended.

Suggestion
Upon discussion with the team, we came to the idea to merge the two properties into one. The new API will be as follows:

public class ListViewGroup 
{
    public enum ListViewGroupCollapsedState 
    {
        DEFAULT,
        EXPANDED,
        COLLAPSED
    }

    public ListViewGroupCollasedState CollapsedState { get; set; }
}
@lonitra lonitra added the api-suggestion (1) Early API idea and discussion, it is NOT ready for implementation label Apr 16, 2020
@weltkante
Copy link
Contributor

What impact will it have on accessibility?

It will be needed to be presented though accessibility API that you can toggle the collapsed state of a group.

@merriemcgaw merriemcgaw added this to the 5.0 milestone Apr 16, 2020
@merriemcgaw merriemcgaw changed the title Push for Adding Collapse Support to ListViewGroup Add Collapse Support to ListViewGroup Apr 16, 2020
@RussKie RussKie modified the milestones: 5.0 Previews 1-4, 5.0 Apr 20, 2020
@merriemcgaw
Copy link
Member

We'll need to see if we can expose the UIA Expand/Collapse pattern. You can chat with @M-Lipin for suggestions when you're at that point.

@RussKie
Copy link
Member

RussKie commented Apr 23, 2020

I've scanned the neighbouring API and the ListView et al API have the following public surface:

so the proposal to add the following fits right in:

  • bool ListViewGroup.Collapsible
  • bool ListViewGroup.Collapsed.

It should be noted that TreeNode appears to favour IsXyz naming convention, that feels inconsistent for the current context:

I believe we should follow the former naming than the latter.

@RussKie RussKie added the api-ready-for-review (2) API is ready for formal API review; applied by the issue owner label Apr 23, 2020
@terrajobst terrajobst added api-approved (4) API was approved in API review, it can be implemented and removed api-ready-for-review (2) API is ready for formal API review; applied by the issue owner labels Apr 23, 2020
@ghost ghost added the 🚧 work in progress Work that is current in progress label Apr 29, 2020
@terrajobst
Copy link
Member

terrajobst commented May 14, 2020

After implementing Collapsible/Collapsed property for ListViewGroup as done so in #3155 , we realize that this will be an issue with the design-time serializer, which orders properties assignment statement alphabetically.

When we approved it, I wrote this in email:

Please don’t throw exceptions when Collapsible == false and Collapsed is set. Properties should be settable independently. Ideally, Collapsed is a no-op when Collapsible is false, but as soon as Collapsible is set to true, the state currently stored in Collapsed is respected.

The mental model I had in mind was something simple like this:

public bool Collapsible 
{
    get => _collapsible;
    set
    {
        if (_collapsible != value)
        {
            _collapsible = true;
            UpdateGroupState();
        }
    }
}

public bool Collapsed
{
    get => _collapsed;
    set
    {
        if (_collapsed != value)
        {
            _collapsed = true;
            UpdateGroupState();
        }
    }
}

private void UpdateGroupState()
{
    // Use _collapsible && _collapsed
}

@RussKie
Copy link
Member

RussKie commented May 15, 2020

The mental model I had in mind was something simple like this:

public bool Collapsible 
{
    get => _collapsible;
    set
    {
        if (_collapsible != value)
        {
            _collapsible = true;
            UpdateGroupState();

Yes, it is something I had in mind too.
However as we started exploring the designer story we come to realisation that correlated properties pose challenge to the designer. It was also uncovered that we completely overlooked the events associated with the properties.

The new proposal is to meld the two properties into one, thus reducing the API surface and make it easier for a developer:

public enum CollapsedState 
{
    None = 0,
    Expanded,    // collapsible
    Collapsed,   // collapsible | collapsed
}

public CollapsedState CollapsedState
{
    get => _collapsedState;
    set
    {
        if (_collapsedState!= value)
        {
            _collapsedState = value;
            OnCollapsedStateChanged(value);
            UpdateGroupState();
        }
    }
}

public EventHandler<ListViewGroupCollapsedStateArgs> CollapsedStateChanged(ListViewGroupCollapsedStateArgs e);

lonitra added a commit that referenced this issue May 28, 2020
Fixes #3067

ListViewGroup originally was not collapsible and thus items in the group
could not be temporarily hidden. These changes add a new property to
ListViewGroup which controls its appearance e.g. whether the group is
collapsible and if the group is in its collapsed state or expanded
state. These changes also raise an event for when the CollapsedState
of a group is changed.
lonitra added a commit that referenced this issue May 29, 2020
Fixes #3067

ListViewGroup originally was not collapsible and thus items in the group
could not be temporarily hidden. These changes add a new property to
ListViewGroup which controls its appearance e.g. whether the group is
collapsible and if the group is in its collapsed state or expanded
state. These changes also raise an event for when the CollapsedState
of a group is changed.
RussKie pushed a commit that referenced this issue May 31, 2020
Fixes #3067

ListViewGroup originally was not collapsible and thus items in the group
could not be temporarily hidden. These changes add a new property to
ListViewGroup which controls its appearance e.g. whether the group is
collapsible and if the group is in its collapsed state or expanded
state. These changes also raise an event for when the CollapsedState
of a group is changed.
@ghost ghost removed the 🚧 work in progress Work that is current in progress label May 31, 2020
@RussKie RussKie removed this from the 5.0 milestone Jul 16, 2020
@ghost ghost locked as resolved and limited conversation to collaborators Feb 1, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api-approved (4) API was approved in API review, it can be implemented api-suggestion (1) Early API idea and discussion, it is NOT ready for implementation
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants