-
Notifications
You must be signed in to change notification settings - Fork 57
button to collapse/expand attributes and attribute groups #78
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
base: master
Are you sure you want to change the base?
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -3,6 +3,11 @@ | |
| /* Items List */ | ||
| /* Attributes */ | ||
| } | ||
| .worldbuilding a.fa-caret-right, | ||
| .worldbuilding a.fa-caret-down { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We should give these a more targeted selector so they don't affect other uses of these icons. Something like
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
| font-weight: 900; | ||
| font-size: smaller; | ||
| } | ||
| .worldbuilding .window-content { | ||
| height: 100%; | ||
| padding: 5px; | ||
|
|
@@ -215,6 +220,10 @@ | |
| border-radius: 2px; | ||
| padding: 2px 5px; | ||
| } | ||
| .worldbuilding .attributes-header .attribute-control:first-child, | ||
| .worldbuilding .group-header .attribute-control:first-child { | ||
| flex: none; | ||
| } | ||
| .worldbuilding .group-key, | ||
| .worldbuilding .group-label { | ||
| font-weight: bold; | ||
|
|
@@ -235,6 +244,9 @@ | |
| flex: 0 0 116px; | ||
| margin: 0 5px; | ||
| } | ||
| .worldbuilding .attribute-collapsed { | ||
| display: none; | ||
| } | ||
| .worldbuilding.sheet.actor { | ||
| min-width: 560px; | ||
| min-height: 420px; | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -91,15 +91,17 @@ <h4 class="item-name">{{item.name}}</h4> | |
| {{!-- 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> | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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?
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Having an expand/collaspe toggle to the left of "Attribute Key" in the main table header is confusing to me.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
| <span class="attribute-key">{{localize "SIMPLE.AttributeKey"}}</span> | ||
| <span class="attribute-value">{{localize "SIMPLE.AttributeValue"}}</span> | ||
| <span class="attribute-label">{{localize "SIMPLE.AttributeLabel"}}</span> | ||
| <span class="attribute-dtype">{{localize "SIMPLE.AttributeDtype"}}</span> | ||
| <a class="attribute-control" data-action="create" data-group="{{group}}"><i class="fas fa-plus"></i></a> | ||
| <input class="attribute-collapsed" type="checkbox" name="system.attributes_collapsed" {{#if systemData.attributes_collapsed}}checked{{/if}} /> | ||
| </header> | ||
|
|
||
| {{!-- Render the attribute list partial. --}} | ||
| {{> "systems/worldbuilding/templates/parts/sheet-attributes.html" attributes=systemData.ungroupedAttributes dtypes=dtypes}} | ||
| {{> "systems/worldbuilding/templates/parts/sheet-attributes.html" attributes=systemData.ungroupedAttributes dtypes=dtypes collapsed=systemData.attributes_collapsed}} | ||
|
|
||
| {{!-- Render the grouped attributes partial and control. --}} | ||
| <div class="groups"> | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All variables should be camel-case i.e.
attributesCollapsedThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry habitual snake case, hard to shake. :)