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

[#1405] Add TraitAdvancement & TraitConfig #2522

Merged
merged 2 commits into from
Nov 1, 2023

Conversation

arbron
Copy link
Collaborator

@arbron arbron commented Oct 25, 2023

Adds the TraitAdvancement type and the configuration application.

Screenshot 2023-10-25 at 12 26 57

@arbron arbron added this to the D&D5E 2.4.0 milestone Oct 25, 2023
@arbron arbron requested a review from Fyorl October 25, 2023 19:26
@arbron arbron self-assigned this Oct 25, 2023
@arbron arbron mentioned this pull request Oct 29, 2023
7 tasks
Copy link
Contributor

@Fyorl Fyorl left a comment

Choose a reason for hiding this comment

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

Thanks for all the hard work on this, Jeff. This is a very complex configuration workflow that you've managed to represent in a very compact way.

I had a little bit of trouble really understanding what was going on and how it worked at first, but I got the hang of it eventually and it makes sense. I think since we view configuring custom advancements more of an advanced tool, we can tolerate a higher level of complexity in the UI.

I had some suggestions for ways we might be able to make the dialog a little more intuitive initially though. The radio buttons threw me off at first since they seemed to imply I was making some configuration choice, but actually I was just toggling between which set I was configuring at a time. One option that came to mind was doing away with the in-place editing entirely. We would essentially move the right-hand side panel inside its own dialog that is spawned when you choose to configure a given set by clicking on its cog icon.

image

You can add and remove sets as before, then click the cog to configure which items appear in each set.

This would allow us to move the 'count' configuration inside this new dialog too. I also struggled a bit with the count as it was part of the same fieldset, and wasn't immediately clear that I was configuring a count for the selected set rather than some sort of count for the total number of options or something.

I appreciate it would be a significant rewrite so I'm not sure how palatable a suggestion it is, but I thought I would throw it out there in case it resonated strongly with you.

If we don't want to go with that suggestion, another idea I had was moving the count configuration 'inline' with the set description. So each one would always read at least '1 from X, Y, Z', where the '1' is actually inside its own inline number input. That would make the count configuration a lot clearer to me at least, though I appreciate it complicates the form handling somewhat.

Similarly, if we do not want to go with the separate dialogs suggestion, then I think my main issue was with the radio buttons. If we had some alternative method of highlighting the 'selected' set that wasn't a radio button (some bespoke CSS styling might do), and somehow connecting that to the fields that are used to configure the selected set (the checkboxes on the right, and the 'count' configuration), then I think it would be an improvement too.

module/applications/advancement/trait-config.mjs Outdated Show resolved Hide resolved
module/applications/advancement/trait-config.mjs Outdated Show resolved Hide resolved
templates/advancement/trait-config.hbs Outdated Show resolved Hide resolved
module/documents/advancement/trait.mjs Show resolved Hide resolved
module/documents/advancement/trait.mjs Outdated Show resolved Hide resolved
@arbron
Copy link
Collaborator Author

arbron commented Nov 1, 2023

@Fyorl I've made a pass at changing the styling of the sheet:

Screenshot 2023-10-31 at 17 11 34

I moved Count to the right column with the trait choices, replaced the radio buttons with a cog and arrow for selected. I've also made the selected option bold for better distinction.

I swapped out the minus icon for a trash can because the minus icon felt a bit too visually similar to the arrow.

I increase the gap between the two columns to give them some visual separation.

@arbron arbron requested a review from Fyorl November 1, 2023 00:16
Copy link
Contributor

@Fyorl Fyorl left a comment

Choose a reason for hiding this comment

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

I think the changes address most of the major points of friction I had, thanks for making those. Especially the tools list gets very long and unwieldy so I hope we could have some better way of displaying these trait options eventually. That applies to not only this dialog but also the trait dialogs on the character sheet.

I've used checkbox grids to good effect in other designs, but they only really allow for one 'layer' of category nesting. That should be fine for the core dnd5e traits that we support, but it won't generalise to a full tree unfortunately.

image

Either way, I think the design you've implemented currently is good and we can move ahead with it and iterate on it further later if we feel it's necessary.

@arbron arbron merged commit 6e38b9f into foundryvtt:2.4.x Nov 1, 2023
@arbron arbron deleted the advancement/trait-config branch November 1, 2023 14:17
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.

None yet

2 participants