Add collapsable section for option groups #736

Glavin001 · 2016-02-21T22:32:27Z

About

For a package that has a lot of options, such as Atom Beautify, it appears to be very difficult for users to find the options they want.

I propose that the groups be collapsable.

Features

Groups of options are collapsable
- Open by default
Force group to be collapsed

{
  group1: {
    title: 'Group 1',
    type: 'object',
    collapsed: true
  }
}

~~"Collapse all sections" button (like the styleguide has)~~
- ~~Only show "Collapse All" button when there are collapsable sub-sections~~

Screenshots

Open Group 1:

Collapsed Group 1:

Options used

  _group1: {
    title: 'Group 1',
    type: 'object',
    order: 1,
    properties: {
      prop1: {
        type: 'integer',
        title: 'Prop1',
        default: 1
      },
      prop2: {
        title: 'Prop2'
        default: 1
      },
    },
  },
  _group2: {
    title: 'Group 2',
    type: 'object',
    order: 2,
    properties: {
      prop1: {
        type: 'integer',
        title: 'Prop1',
        default: 1
      },
      prop2: {
        title: 'Prop2'
        default: 1
      },
    },
  }

/cc #597 Glavin001/atom-beautify#620 Glavin001/atom-beautify#405

Glavin001 · 2016-02-21T22:37:31Z

Please let me know if there is anything I can do to make this merged and published quicker.

I'll take a look at the Travis CI failed tests once I am home.

Example usage: ```javascript barGroup: # Expanded by default type: 'object' title: 'Bar group' properties: bar: title: 'Bar' description: 'The bar setting' type: 'boolean' default: false bazGroup: # Collapsed by default type: 'object' collapsed: true # field to force collapse a grouped setting properties: baz: title: 'Baz' description: 'The baz setting' type: 'boolean' default: false ```

Glavin001 · 2016-02-22T04:02:39Z

I have added tests and the ability to force a grouped setting to be collapsed by default.

Example usage:

    barGroup:   # Expanded by default
        type: 'object'
        title: 'Bar group'
        properties:
          bar:
            title: 'Bar'
            description: 'The bar setting'
            type: 'boolean'
            default: false
    bazGroup:   # Collapsed by default
        type: 'object'
        collapsed: true # field to force collapse a grouped setting
        properties:
          baz:
            title: 'Baz'
            description: 'The baz setting'
            type: 'boolean'
            default: false

Waiting on Atom/settings-view support: atom/settings-view#736

thedaniel · 2016-02-22T09:30:29Z

@simurai do you have feelings about this?

jerone · 2016-02-22T12:10:29Z

With many groups, what about a "Collapse/expand all sections" (like the styleguide has)...

Glavin001 · 2016-02-22T14:21:25Z

With many groups, what about a "Collapse/expand all sections" (like the styleguide has)...

I'll look into that, too. Good idea. Thanks.

Glavin001 · 2016-02-22T15:20:56Z

Collapse All button

Before: Expanded sections

After: Collapsed all

Glavin001 · 2016-02-22T15:36:15Z

The failing test on Travis CI is not influenced by my changes and only happens intermittently.

Glavin001 · 2016-02-22T17:10:28Z

Please restart the Travis CI build to see that the tests do pass.

Someone should probably fix whatever is causing the timeout error:

PackageDetailView
  it displays the snippets registered by the package
    timeout: timed out after 60000 msec waiting for something to happen

simurai · 2016-02-23T13:31:28Z

I think it's great. Maybe not relevant for most packages, but looking at atom-beautify, I can see the need for it. 😆

Should the "Collapse All Sections" button be an "Expand All Sections" button instead. For atom-beautify it probably makes more sense to collapse all sections by default, except maybe "General". Then you just open the ones you're interested in. vs. "ohh this is overwhelming, let's first collapse everything".

In that case, the button could be disabled, once everything is expanded.

Some packages might start to show the most essential settings ungrouped and then a collapsed "Advanced settings" group that is a bit more hidden for less common settings. I think that would be a nice pattern.

Glavin001 · 2016-02-23T14:35:08Z

I have work and a midterm to study for so it is unlikely I will be making any changes until at the earliest Thursday night.

That gives everyone some time to chime in, discuss, and settle on exactly what they want so that this can be reviewed and merged ASAP.

Questions to answer/decide:

Should it be a "Collapse All" or "Expand All" button?
Is the default to have groups be collapsed or expanded? (currently default is expanded and you use collapsed:true to force group being collapsed by default)
Any implementation changes?
Any User Interface changes?

Thank you.

Glavin001 · 2016-02-25T15:05:50Z

Any review of this pull request? Is there anything that needs to be changed before this can be merged? Thank you.

Glavin001 · 2016-02-29T02:37:24Z

Any status on review of this pull request? I know there are at least many Atom Beautify users who are excited for this. Thanks in advance for merging 😜.

thedaniel · 2016-02-29T11:14:33Z

Regardless of the default, in your screenshot the "Collapse all" button still says "Collapse" even after all the sections are collapsed. It should be "Expand" in that state.
What does a mix of grouped and ungrouped settings render like in this state? (Is that even possible with our config schema?)

Glavin001 · 2016-02-29T14:20:50Z

Regardless of the default, in your screenshot the "Collapse all" button still says "Collapse" even after all the sections are collapsed. It should be "Expand" in that state.

This is the same as how it is implemented in the Styleguide. It also shows "Collapse All Sections" when all sections are already collapsed:

Clicking "Collapse All Sections" toggles the individual collapse classes on each of the .sub-sections.
See https://github.com/Glavin001/settings-view/blob/master/lib/settings-panel.coffee#L84
If we want it to show Expand then we would have to observe the changes to all .sub-section elements and check if each of them are collapsed.

I think that while the "Collapse" / "Expand" functionality is a good idea, since other views, such as Stlyeguide, already have a simpler "Collapse All Sections" functionality that this should be a separate Issue. If we want "Collapse" / "Expand" then that should be another Pull Request specifically targeting the overall functionality of Collapsing and add support for Expanding.

If this is still a large concern, then I would be forced to suggest that we remove the "Collapse All Sections" button in this Pull Request all together and someone can commit the "Collapse" / "Expand" functionality on their own. This Pull Request is specifically targeting the collapsable sub-sections for within Settings views. The "Collapse All Sections" button was a nice-to-have that I thought would help create excitement and encourage this Pull Request to be closed quicker. However, if that is not the case, then I will promptly remove it in hopes that the core feature, collapsable settings, will be merged promptly.

What does a mix of grouped and ungrouped settings render like in this state? (Is that even possible with our config schema?)

See #736 (comment) for screenshot.

Glavin001 · 2016-03-04T18:07:34Z

Please let me know if I should make a new Pull Request with only the Collapse buttons for each sub-section -- the core purpose of this Pull Request -- and remove the seemingly controversial Collapse All Sections button that is feature creeping for this Pull Request. Thank you.

prettydiff · 2016-03-10T00:24:18Z

+1

simurai · 2016-03-10T06:15:17Z

The Styleguide doesn't necessarily have to serve as the blueprint for this. I think over time the list became too long and then a button got added to collapse everything so you don't have to scroll too much. But personally I would prefer if all the sections in the Styleguide are already collapsed initially.

If this is still a large concern, then I would be forced to suggest that we remove the "Collapse All Sections" button in this Pull Request

👍 Ya, maybe a good idea to find agreement in a separate PR.

Glavin001 · 2016-03-10T06:34:19Z

Given this Pull Request has taken this long to review, I have simplified it to hopefully make it more appealing so it can be merged sooner.

This Pull Request now only adds the following functionality:

Collapsable groups
Force group be start collapsed with option schema
Options within group are sorted
New specs for this functionality (and Travis CI says they pass!)

Thank goodness for atomic commits and rebasing.

simurai · 2016-03-15T00:08:08Z

I tested this in the "Editor Settings" to toggle the Invisibles group and it works as expected. 👍

Maybe some final 👀 on the code would be great.

Glavin001 · 2016-03-16T22:11:28Z

Is there anything else I can do for this issue to be reviewed by those who have merge permission? I thought adding specs passing Travis CI would help expedite this process. Please let me know how else I can help this move along so Atom and Atom Beautify users can benefit. Thank you!

thedaniel · 2016-03-17T13:30:32Z

lib/settings-panel.coffee

@@ -67,7 +70,7 @@ class SettingsPanel extends View
            appendSetting.call(this, namespace, name, settings[name])

  sortSettings: (namespace, settings) ->
-    _.chain(settings).keys().sortBy((name) -> name).sortBy((name) -> atom.config.getSchema("#{namespace}.#{name}")?.order).value()
+    sortSettings(namespace, settings)


Just curious, why did you move this out into a separate function since it's only used here?

I use sortSettings in two places:

https://github.com/Glavin001/settings-view/blob/master/lib/settings-panel.coffee#L62

https://github.com/Glavin001/settings-view/blob/master/lib/settings-panel.coffee#L305

I moved the sortSettings functionality out since I use sortSettings outside of the SettingsPanel class at https://github.com/Glavin001/settings-view/blob/master/lib/settings-panel.coffee#L294-L307

This function could be removed and code changed from @sortSettings to simply calling sortSettings: https://github.com/Glavin001/settings-view/blob/master/lib/settings-panel.coffee#L72-L73

sortSettings: (namespace, settings) -> sortSettings(namespace, settings)

However, now I look at

settings-view/spec/settings-panel-spec.coffee

Lines 42 to 50 in e9ac721

sortedSettings = settingsPanel.sortSettings("foo", settings)

expect(sortedSettings[0]).toBe 'zing'

expect(sortedSettings[1]).toBe 'zang'

expect(sortedSettings[2]).toBe 'bar'

expect(sortedSettings[3]).toBe 'gong'

expect(sortedSettings[4]).toBe 'haz'

it "gracefully deals with a null settings object", ->

sortedSettings = settingsPanel.sortSettings("foo", null)

and see there are spec that use settingsPanel.sortSettings function.

thedaniel · 2016-03-17T13:30:51Z

I left one comment otherwise I think it is good to merge.

Glavin001 · 2016-03-17T13:56:05Z

Could someone restart the Travis CI build? Something appears to have gone wrong. It does not look like it ever reached running the specs.

thedaniel · 2016-03-17T14:07:02Z

OK, I really appreciate your patience! This should help with the pathological case of config size without changing the experience of the 99% case. It's just in time to get into the 1.7 beta this week.

Add collapsable section for option groups

Glavin001 · 2016-03-17T14:09:59Z

Awesome!!! Thank you very much for reviewing and merging. Happy to contribute to Atom.

Add collapsable section for option groups

c1f6600

50Wliu added the needs-review label Feb 21, 2016

Glavin001 added a commit to Glavin001/atom-beautify that referenced this pull request Feb 22, 2016

Organize settings/options into groups that will be collapsable

f2968b7

Waiting on Atom/settings-view support: atom/settings-view#736

Glavin001 force-pushed the master branch from 5af516c to dfd3848 Compare Feb 23, 2016

simurai mentioned this pull request Feb 23, 2016

Settings collapse button will be too big atom/one-dark-ui#126

Closed

lee-dohm added the atom label Feb 29, 2016

Glavin001 mentioned this pull request Mar 9, 2016

Beautify settings UX Glavin001/atom-beautify#713

Closed

Allow ordering of settings within object field

c3fefca

Glavin001 force-pushed the master branch from dfd3848 to c3fefca Compare Mar 10, 2016

thedaniel reviewed Mar 17, 2016
View changes

Merge branch 'master' of github.com:atom/settings-view

af54cf2

thedaniel added a commit that referenced this pull request Mar 17, 2016

Merge pull request #736 from Glavin001/master

5e6f8b4

Add collapsable section for option groups

thedaniel merged commit 5e6f8b4 into atom:master Mar 17, 2016

Glavin001 mentioned this pull request Mar 21, 2016

Better settings and performance Glavin001/atom-beautify#864

Merged

13 tasks

Nov	DEC	Jan
	07
2021	2022	2023

Add collapsable section for option groups #736

Add collapsable section for option groups #736

Glavin001 commented Feb 21, 2016

Glavin001 commented Feb 21, 2016

Glavin001 commented Feb 22, 2016

thedaniel commented Feb 22, 2016

jerone commented Feb 22, 2016

Glavin001 commented Feb 22, 2016

Glavin001 commented Feb 22, 2016

Glavin001 commented Feb 22, 2016

Glavin001 commented Feb 22, 2016

simurai commented Feb 23, 2016

Glavin001 commented Feb 23, 2016

Glavin001 commented Feb 25, 2016

Glavin001 commented Feb 29, 2016

thedaniel commented Feb 29, 2016

Glavin001 commented Feb 29, 2016

Glavin001 commented Mar 4, 2016

prettydiff commented Mar 10, 2016

simurai commented Mar 10, 2016

Glavin001 commented Mar 10, 2016

simurai commented Mar 15, 2016

Glavin001 commented Mar 16, 2016

thedaniel Mar 17, 2016

Glavin001 Mar 17, 2016

thedaniel commented Mar 17, 2016

Glavin001 commented Mar 17, 2016

thedaniel commented Mar 17, 2016

Glavin001 commented Mar 17, 2016

	sortedSettings = settingsPanel.sortSettings("foo", settings)
	expect(sortedSettings[0]).toBe 'zing'
	expect(sortedSettings[1]).toBe 'zang'
	expect(sortedSettings[2]).toBe 'bar'
	expect(sortedSettings[3]).toBe 'gong'
	expect(sortedSettings[4]).toBe 'haz'

	it "gracefully deals with a null settings object", ->
	sortedSettings = settingsPanel.sortSettings("foo", null)

Add collapsable section for option groups #736

Add collapsable section for option groups #736

Conversation

Glavin001 commented Feb 21, 2016

About

Features

Screenshots

Options used

Glavin001 commented Feb 21, 2016

Glavin001 commented Feb 22, 2016

thedaniel commented Feb 22, 2016

jerone commented Feb 22, 2016

Glavin001 commented Feb 22, 2016

Glavin001 commented Feb 22, 2016

Collapse All button

Before: Expanded sections

After: Collapsed all

Glavin001 commented Feb 22, 2016

Glavin001 commented Feb 22, 2016

simurai commented Feb 23, 2016

Glavin001 commented Feb 23, 2016

Glavin001 commented Feb 25, 2016

Glavin001 commented Feb 29, 2016

thedaniel commented Feb 29, 2016

Glavin001 commented Feb 29, 2016

Glavin001 commented Mar 4, 2016

prettydiff commented Mar 10, 2016

simurai commented Mar 10, 2016

Glavin001 commented Mar 10, 2016

simurai commented Mar 15, 2016

Glavin001 commented Mar 16, 2016

thedaniel Mar 17, 2016

Choose a reason for hiding this comment

Glavin001 Mar 17, 2016

Choose a reason for hiding this comment

thedaniel commented Mar 17, 2016

Glavin001 commented Mar 17, 2016

thedaniel commented Mar 17, 2016

Glavin001 commented Mar 17, 2016