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

Add mouse shortcuts to skills tab #4373

Merged

Conversation

Dullson
Copy link
Contributor

@Dullson Dullson commented May 17, 2022

I wanted to ease the tedium of enabling/disabling multiple skill groups typically found in build guide pobs and went a little further in the process.

This PR adds the following shortcuts to the skill list in Skills Tab:

  • Right Click: enable/disable skill group
  • ALT + Right Click: include/exclude skill group in FullDPS calculation
  • ALT + Left Click: select skill group as the main skill

also [FullDPS] and [Active] tags are added to skill group labels to indicate their configurations

Current behavior

This is the current process when you want to switch between different skill sets, it requires a lot of clicks:

OAMXmQFHpe.mp4

Also, you have to hunt for enabled FullDPS boxes if you have multiple sets of the same skill since the left column display doesn't include any info other than the active skill name:

eC3zrsd2sC.mp4

Proposed functionality

27YIRkmkVZ.mp4

I was going to split this PR into two parts, one for enable/disable shortcut and another one for other functionality which comes with new tags but didn't want to clutter the repo with similar requests.

Toggling enable/disable already has an indicator ((Disabled) tag) so it is a visually non-invasive addition compared to the other two.

Added tags look fine to me but I can see an argument being made for potential visual clutter. I considered adding background color to rows instead of tags but throwing around colors doesn't feel better.
Between two tags, FullDPS one feels mandatory with hotkey since there is no indicator for toggling this flag like enable/disable tag (Disabled).
Active tag is a helpful indicator in no label scenarios like this and provides instant feedback when main skill is changed via proposed alt+click shortcut but one can argue that it's not mandatory for shortcut function since main skill update on left panel is also visible.

Overall, I think these features feel great whether you navigate a content creator's PoB with a skill section for every act or check numbers on the different combinations of curse-aura-skill setups.

All the styling and hotkey selection are done arbitrarily, let me know if you have any suggestions.

@Wires77 Wires77 added the user-interface Changes that only affect the UI label May 29, 2022
Copy link
Member

@Wires77 Wires77 left a comment

Choose a reason for hiding this comment

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

This looks good! To be consistent, I'm going to redo some of the mappings a bit:

  • Disable/Enable as Ctrl+Click to match Items
  • Right click to set to active
  • Ctrl+RClick to add to FullDPS
  • Also change the brackets to parentheses

Basically nothing uses Alt in PoB, and nothing uses Right Click (which is kind of a shame).
Brackets are only really used for tree versions, so those should probably just stay parentheses, but the colors are fine

@Wires77 Wires77 merged commit 4bf691b into PathOfBuildingCommunity:dev May 29, 2022
Dullson added a commit to Dullson/PathOfBuilding that referenced this pull request May 29, 2022
* feat: add right click toggle enable to skill list

* feat: add FullDPS tag to skill list

* refactor: GetHoverIndex out of GetHoverValue on ListClass

* feat: add Active tag to skill list

* feat: update tips section on skills tab

* Fixing consistency issues and a nil bug

Co-authored-by: Wires77 <[email protected]>
@Dullson Dullson deleted the feature-skill-list-shortcuts branch June 1, 2022 13:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
user-interface Changes that only affect the UI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants