button to collapse/expand attributes and attribute groups #78
Conversation
|
@aaclayton / @Fyorl it was suggested I bring this to your attention. |
aaclayton
left a comment
aaclayton
left a comment
There was a problem hiding this comment.
Good idea, a few changes necessary.
styles/simple.css
Outdated
| .worldbuilding a.fa-caret-right, | ||
| .worldbuilding a.fa-caret-down { |
There was a problem hiding this comment.
We should give these a more targeted selector so they don't affect other uses of these icons. Something like .attribute-control.collapse
There was a problem hiding this comment.
Sorry that shouldn't have been included at all it was from an earlier iteration which didn't use the "fas" class which is now providing the defaults for those values.
templates/actor-sheet.html
Outdated
| {{!-- Attributes Tab --}} | ||
| <div class="tab attributes" data-group="primary" data-tab="attributes"> | ||
| <header class="attributes-header flexrow"> | ||
| <a class="attribute-control" data-action="collapse"><i class="fas {{#if systemData.attributes_collapsed}}fa-caret-right{{else}}fa-caret-down{{/if}}"></i></a> |
There was a problem hiding this comment.
I understand the motivation to collapse specific attribute groups, but I don't understand the collapse button in the main attributes header. I think I would prefer to keep functionality to collapse groups, but not support collapsing the overall attributes list. Thoughts?
There was a problem hiding this comment.
The motivation is the same as for groups to provide a way of hiding the details of a set of attributes to make working with the values of a sheet easier for players. Maybe some groups are rarely changed or only for accounting purposes, players should be able to collapse them to make find the attributes they are interested in easier, or to give more control over how the attributes sheet is presented to them.
I though it best not to make any assumptions about how people were organizing their attributes and make every list of attributes collapsible including the main attributes. It is only collapsing those attributes which are not part of a group, the rest of the sheet, i.e. all the groups, would still be presented.
There was a problem hiding this comment.
Are you thinking that the main attributes should always be shown, or do you think that the role expand button in the top attribute bar is confusing?
There was a problem hiding this comment.
Having an expand/collaspe toggle to the left of "Attribute Key" in the main table header is confusing to me.
There was a problem hiding this comment.
Ok. I would still be in favour of having the top level attributes be collapsible.
Could we maybe give them a special group header just without any value for the group? And allow that to be collapsible?
There was a problem hiding this comment.
I think the ambiguity in my mind is not knowing whether the top-level collapse will collapse the entire tab (ungrouped attributes and groups) or whether it would just hide ungrouped attributes. I presume the implementation is the later, but I think the UX is muddy.
I'm not sure I have much bandwidth for iteration at the moment, so I think your best shot is to remove this top-level collapse and we can focus on getting the rest of this functionality merged, then we could revisit what to do for ungrouped attributes in a follow-up PR.
There was a problem hiding this comment.
Giving the to level attributes their own group header would remove that ambiguity, but I'm happy to just leave the top level attributes as uncollapsible for now.
module/helper.js
Outdated
| } | ||
|
|
||
| // Copy across collapsed flag for attributes | ||
| formAttrs.attributes_collapsed = expanded?.attributes_collapsed; |
There was a problem hiding this comment.
All variables should be camel-case i.e. attributesCollapsed
There was a problem hiding this comment.
Sorry habitual snake case, hard to shake. :)
…oup is supplying the defaults for these
2 participants
Collapsible/expandable attributes/groups, which are remembered for each actor.