Skip to content

Conversation

@Sonic853
Copy link

@Sonic853 Sonic853 commented Jul 3, 2025

I found that the getText and getIcon methods in action_preview.ts use a lot of if judgments. I think changing them to switch will make them more efficient.

case 'PAD_ENTER': return '↵'
case 'PAD_PERIOD': return '.'
}
if (key.startsWith('PAD_')) return key.slice('PAD_'.length)
Copy link
Contributor

@marcos-diaz marcos-diaz Aug 11, 2025

Choose a reason for hiding this comment

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

.slice('PAD_'.length)

This approach is harder to understand at first glance, rather than simply splitting by _

switch (hid) {
case 'KEY_SPACE':
icon = 'space_bar'
break
Copy link
Contributor

Choose a reason for hiding this comment

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

This is the reason I try to avoid switch in JS, too much syntax noise for a minimal (negligible) performance gain.

if (label.startsWith('KEY_')) {
const key = label.slice('KEY_'.length)
switch (key) {
case 'ESCAPE': return 'Esc'
Copy link
Contributor

Choose a reason for hiding this comment

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

I like these (because they do not need break)

if (this.section instanceof CtrlButton) {
if (index==1 && this.section.hold) cls += ' hold'
if (index==2 && this.section.double) cls += ' double'
if (index == 1 && this.section.hold) cls += ' hold'
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

const icon = this.getIcon(action)
const cls = this.getClass(0, action, text, icon)
return {cls, text, icon}
return { cls, text, icon }
Copy link
Contributor

Choose a reason for hiding this comment

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

This flavor of linting is up to personal preference

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