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

[form-builder] Fix bug with fieldsets #514

Merged
merged 1 commit into from Jan 19, 2018
Merged

Conversation

bjoerge
Copy link
Member

@bjoerge bjoerge commented Jan 19, 2018

Something got messed up with the defaults here, so fieldsets ended up being non-collapsible and collapsed by default, which was unfortunate. This fixed it so that the defaults are:

collapsible: true
collapsed: true

@bjoerge bjoerge changed the base branch from next to master January 19, 2018 13:20
@bjoerge bjoerge merged commit 409f28f into master Jan 19, 2018
@bjoerge bjoerge deleted the fix-fieldsets-not-collapsible branch January 19, 2018 13:25
@kristofferjs
Copy link
Contributor

The default should be collapsible: false.
Fieldsets should be non-collapsible as default (and not collapsed).

@bjoerge
Copy link
Member Author

bjoerge commented Jan 19, 2018

Ah, aight. We should probably change that default in next, then.

@kristofferjs
Copy link
Contributor

kristofferjs commented Jan 19, 2018

If i remember coreectly: We set the collapsed based on the level in form-builder. So if there are many nested fieldsets, they will auto-collapse on level > 2 if they are collapsible.

@bjoerge
Copy link
Member Author

bjoerge commented Jan 29, 2018

So there are several cases to consider here:

  1. {collapsed: true} - this should force the fieldset to be collapsible, so this would resolve to {collapsed: true, collapsible: true}
  2. {collapsible: true} - resolve to {collapsible: true, collapsed: false}
  3. {collapsible: false, collapsed: true} - what to do? The field will be hidden and not possible to expand at all. I feel the most sane would be to handle it as {collapsible: true, collapsed: true}
  4. {collapsible: false, collapsed: false} - as is.
  5. {collapsible: true, collapsed: true} - as is.

How should these apply on level > 2?

@kristofferjs
Copy link
Contributor

I think it is best only to handle collapsible if it is true. Ignore collapsed if collapsible is false/undefined.

@kristofferjs
Copy link
Contributor

Level > 2 should be {collapsible: true, collapsed: true} as default. (if we set the levels here)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants