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

New resource bar components w/ keyboard controls #501

Merged
merged 33 commits into from
Oct 7, 2022

Conversation

rsek
Copy link
Collaborator

@rsek rsek commented Oct 6, 2022

introduces a more unified set of components that are friendly to both Actors and Items.

it also fixes the missing "selected" highlight on the starforged theme (not totally sure when it happened, but a player in one of my co-op games pointed it out).

  • the slider role; spinbutton is probably a valid choice here too, TBH, but neither is fully satisfactory. might change my mind again later
  • full keyboard control for slider widgets (when they have focus), generally adhering to WC3 recs
  • readonly mode: renders the same slider as a 'readonly' preview, for use in e.g. the asset browser
  • assertive styling for the slider :focus selector
  • collapse "burn momentum" button into the momentum label (with an explanatory tooltip), which should make things nicer for locales that needed to fit more than 4 characters into the 'Burn' button
  • hovering the burn momentum button highlights the box with the PC's momentum reset value
  • following the style on the SF printable sheet, omits the + signs from resources that don't have negative values (e.g. everything but momentum)
  • replace remaining AssetTrack and Stack instances (where they represent resource bars)
  • fake a text-stroke (at least until chromium fixes the bug) to ensure legibility on highlighted bar segments with the SF theme

ideas for further expansion/refinement:

  • debounce multiple rapid increments/decrements in chat messages
  • compact mode: renders the same slider as a spinner
  • consider cribbing some styling from the SF sheet (the overlapping arrows are kinda fun -- might mean bring SVG in)
  • note keybindings in tooltip -- need to consider the best way to style the tooltips here, as currently they'd be rather large.

@rsek rsek marked this pull request as ready for review October 7, 2022 01:13
Copy link
Owner

@ben ben left a comment

Choose a reason for hiding this comment

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

I like this a lot, just a few things I noticed. And a reminder that the shared-supply feature got left behind.

src/module/vue/components/resource-meter/keybind-def.vue Outdated Show resolved Hide resolved
src/module/features/customassets.ts Show resolved Hide resolved
src/module/vue/components/resource-meter/attr-slider.vue Outdated Show resolved Hide resolved
Comment on lines 35 to 38
<span class="momentum-status-max">
{{ $t('IRONSWORN.Max') }}:
{{ actor?.data.momentumMax }}
</span>
Copy link
Owner

Choose a reason for hiding this comment

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

Maybe now is when can trim this. We already indicate the max by disabling the high numbers, so this is just left over from the paper sheet.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

huh, yeah, the same thing crossed my mind. we might even take it a step further by marking the reset value with an icon of some kind, and ditching that label, too. it's also conveyed in the burn button tooltip.

@@ -13,6 +13,19 @@ body.theme-starforged {
@text-medium-color: #3dd;
@text-light-color: #6ff;

// until this chromium issue with paint-order is resolved, text-stroke is functionally useless: https://bugs.chromium.org/p/chromium/issues/detail?id=815111
// yes, i am bitter, because even safari has supported it for ~4 years at this point :)
Copy link
Owner

Choose a reason for hiding this comment

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

🌶️ 😀

Copy link
Owner

@ben ben left a comment

Choose a reason for hiding this comment

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

Getting closer, just a few more comments

src/module/vue/shared-sheet.vue Show resolved Hide resolved
src/module/helpers/settings.ts Show resolved Hide resolved
@rsek rsek mentioned this pull request Oct 7, 2022
37 tasks
Copy link
Owner

@ben ben left a comment

Choose a reason for hiding this comment

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

LGTM, merge whenever you're ready.

@rsek rsek merged commit 64c8b2f into ben:main Oct 7, 2022
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.

2 participants