Skip to content

Conversation

@interim17
Copy link
Contributor

Time estimate or Size

small

NavButton and ViewportButton use a custom prop of clickHandler which is essentially the same as the antd Button's native onClick

More symmetry in the props of different buttons (basic antd as well as our wrappers, and the <button> they eventually render in the DOM), is useful when managing focus and click/keyboard navigation behavior.

I don't recall a justification for using clickHandler instead of just onClick that I can remember so I'd like to switch them all to onClick.

@interim17 interim17 requested a review from a team as a code owner November 15, 2024 17:24
@interim17 interim17 requested review from toloudis and tyler-foster and removed request for a team November 15, 2024 17:24
@github-actions
Copy link

Coverage report

St.
Category Percentage Covered / Total
🟡 Statements 66.92% 696/1040
🟡 Branches 67.1% 104/155
🔴 Functions 35.5% 93/262
🟡 Lines 65.3% 619/948

Test suite run success

129 tests passing in 7 suites.

Report generated by 🧪jest coverage report action from df11ed3

Copy link
Contributor

@toloudis toloudis left a comment

Choose a reason for hiding this comment

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

I don't know technically why onClick is better than clickHandler or whether there's any difference at all. Apart from that, if everything's working, I approve!

@interim17
Copy link
Contributor Author

I don't know technically why onClick is better than clickHandler or whether there's any difference at all. Apart from that, if everything's working, I approve!

In previous code no practical difference, but if for example you want to inject functions or otherwise work with different types of buttons, and some of them have clickHandler and some don't it adds complexity. It's nice if their props are the same whenever possible.

Realizing this as I work on keyboard navigation and wanted to just get ahead of it.

@interim17 interim17 merged commit 2709efe into main Nov 28, 2024
6 checks passed
@interim17 interim17 deleted the feature/button-click branch November 28, 2024 00:01
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.

4 participants