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 all/expand all to Listcontrol #912

Merged
merged 9 commits into from
Dec 22, 2017
Merged

Add collapse all/expand all to Listcontrol #912

merged 9 commits into from
Dec 22, 2017

Conversation

drlogout
Copy link
Contributor

- Add collapse all/expand all to Listcontrol

This adds a button to collapse and expand all list items without closing the whole list widget.

To sort list items with drag & drop it's more practical if they are collapsed. Without a collapse all/expand all switch you need to collapse every item manually.

collapse-expand-all

@verythorough
Copy link
Contributor

verythorough commented Dec 10, 2017

Deploy preview ready!

Built with commit af30171

https://deploy-preview-912--netlify-cms-www.netlify.com

@verythorough
Copy link
Contributor

verythorough commented Dec 10, 2017

Deploy preview ready!

Built with commit af30171

https://deploy-preview-912--cms-demo.netlify.com

Copy link
Contributor

@Benaiah Benaiah left a comment

Choose a reason for hiding this comment

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

When all the items are collapsed/expanded manually, the "collapse/expand all" button should detect that and switch its functionality accordingly. e.g., if you collapse all the items manuallythe button should automatically switch to "expand all".


for (let i = 0; i < itemsCount; i++) {
itemsCollapsed = itemsCollapsed.set(i, !allItemsCollapsed);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

These last three lines can be replaced with something like

const newItemsCollapsed = Array(itemsCollapsed.length).fill(!allItemsCollapsed);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point! The button updates it's state accordingly now.

But I sticked with the forloop, because I couldn't find a more slick solution. With the loop I can simultaneously create and set the list elements.

Copy link
Contributor

@Benaiah Benaiah Dec 18, 2017

Choose a reason for hiding this comment

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

This can remain as-is - @erquhart pointed out that replacing this is more involved than I thought and the for-loop is a pretty simple solution here.

@erquhart
Copy link
Contributor

@drlogout awesome work. I pushed a commit to update the design, good to merge.

@drlogout
Copy link
Contributor Author

Regarding issue #928 list items are now collapsed by default. They can be expanded by adding expandItems: true to the config.yml.
I also added a switch to specify the collapsed state of the whole list: collapsed: true.

      - name: "authors"
        label: "Authors"
        file: "_data/authors.yml"
        description: "Author descriptions"
        fields:
          - name: authors
            label: Authors
            widget: list
            collapsed: true
            expandItems: true
            fields:
              - {label: "Name", name: "name", widget: "string"}
              - {label: "Description", name: "description", widget: "markdown"}

What do you think?

I needed to change field: PropTypes.node to field: PropTypes.object and I'm not sure if this is correct.

@Benaiah
Copy link
Contributor

Benaiah commented Dec 18, 2017

@drlogout so collapsed is the default for the whole list, but expandItems is the default for the items within it? I'd prefer if that terminology was more consistent - maybe collapse and collapseItems?

EDIT: after re-reviewing, I'm ready to approve this PR once we've resolved the names of these options and the PropTypes issue. I'll do a quick look over the final version, but this looks generally solid to me. Thanks so much for the hard work and quick responses, @drlogout!

@erquhart
Copy link
Contributor

I didn't even notice the new field option - good idea @drlogout!

So I discussed this with our designer friend @neutyp and he had a great point: collapsing the list itself isn't super useful to begin with - collapsing the children is really all we need. I agree with that, and I'm thinking we can go ahead and make "collapse/expand" here just refer to the children and remove the old collapse functionality.

Regarding the field option name, I believe we should default these to collapsed when an entry is opened, so I'm thinking "expanded" is a good name for the option.

@t1merickson
Copy link

Thanks for noting our conversation @erquhart — the original intent of the collapse action at the left of the 'function toolbar' in the list is meant to collapse all the children (the list items), not the parent (the whole list itself). I didn't have time to correct our incorrect implementation as we scrambled to hit the 1.0 launch date.

We discussed and agreed on this functionality, but left open the idea of the icon changing in some way to better reflect the functionality. Do you still think an icon change is needed Shawn?

@erquhart
Copy link
Contributor

I've added the double chevron to the code base, but let's just use the standard one - I'll push a commit to update, and that will bring it in line with the addition of object widget collapse in #927, (also by @drlogout 🙌).

@tech4him1 tech4him1 mentioned this pull request Dec 21, 2017
@erquhart
Copy link
Contributor

@drlogout quick summary of my changes:

  • Remove collapsing of the list itself, leaving only collapsing of all children
  • Derive whether all items are collapsed rather than storing that separately in state
  • Use Array(length).fill(value) to set collapsed items on allItemsCollapsed toggle, rather than for loop

Thanks again for getting this in!

@erquhart erquhart dismissed Benaiah’s stale review December 22, 2017 21:29

Dismissing since Benaiah's concern was addressed.

@erquhart erquhart merged commit 6ca9c04 into decaporg:master Dec 22, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants