feat: keyboard button builders #420

KnorpelSenf · 2023-05-31T13:28:39Z

Adds keyboard button builders.

static method to create buttons from labels via Keyboard.text("label")
static method to create keyboards from buttons via Keyboard.from(array)
static method to create keyboards from labels via Keyboard.from(stringArray)
instance method to clone keyboards via keyboard.clone()
instance method to append keyboards via keyboard.append(other0, other1, other2)
instance method to transpose rows and columns via keyboard.toTransposed()
instance method to reflow buttons via keyboard.toWrapped()

The same works for inline keyboards.

Closes #104.

Loskir · 2023-05-31T13:42:41Z

Some nitpicks:

I can't see why an option to specify the maximum number of buttons in the first row in reflow is particularly useful. Are there any use-cases?
I don't think reflow should be an in-place method. It should rather be a static method receiving a 1-d array of buttons. Maybe name it chunk as well
I see that it follows a common theme, but it annoys me so much that transpose is an in-place method.
Is fromColumns any useful? Seems like a very specific method too.

Loskir · 2023-05-31T13:44:01Z

Maybe add prepend as well?

Loskir · 2023-05-31T13:47:21Z

Maybe add addRow as an alias for row? So that

const keyboard = new Keyboard()
keyboard.addRow(....)

sounds more natural

KnorpelSenf · 2023-05-31T13:54:00Z

I can't see why an option to specify the maximum number of buttons in the first row in reflow is particularly useful. Are there any use-cases?

Yes. By default, the last row will be half. By allowing the number of buttons on the first row to be specified, we can make the first row half. For 10 items, we can do Keyboard.from(array).reflow(3, { first: array.length % 3 }) which will give us

[  0  ]
[1 2 3]
[4 5 6]
[7 8 9]

rather than

[0 1 2]
[3 4 5]
[6 7 8]
[  9  ]

I don't think reflow should be an in-place method. It should rather be a static method receiving a 1-d array of buttons. Maybe name it chunk as well

I added clone for these cases so one can do keyboard.clone().reflow() if in-place is a problem. How about we remove clone and always return copies of the current keyboard? I see no benefit of static methods here.

I expected Keyboard.from(data).reflow() to be the most common use case, hence in-place made sense.

I see that it follows a common theme, but it annoys me so much that transpose is an in-place method.

Same.

Is fromColumns any useful? Seems like a very specific method too.

It's useful if you want to map an array of strings to a list of buttons that all are places in a single column. I encouter this quite often with Keyboard.fromColumns(["Welcome", "Settings", "About"]). Not providing this method either requires an explicit transpose call or a .map(x => [x]).

KnorpelSenf · 2023-05-31T13:55:05Z

Maybe add prepend as well?

I see no benefit, just flip the two values?

By the way, should this return a copy, too? In other words, should we create concat rather than append?

KnorpelSenf · 2023-05-31T13:55:23Z

Maybe add addRow as an alias for row?

What is the benefit of redundancy?

Loskir · 2023-05-31T14:00:10Z

Maybe add addRow as an alias for row?

What is the benefit of redundancy?

just like you added fromRows that is the same as from.

The use cases are shifting from chaining new Keyboard().text().row() to separate calls

const keyboard = Keyboard.from(initialData)
if (something) {
  keyboard.row(someConditionalData)
}

If this was new code, it would make a lot more sense to call it addRow instead of row. So why not add it to make such code more readable?

Loskir · 2023-05-31T14:04:05Z

Maybe add prepend as well?

I see no benefit, just flip the two values?

You're right, prepend is not that useful and it would also lead to worse code in general.

Loskir · 2023-05-31T14:05:05Z

Forget about addRow, I just realized it accepts row(button, button, button) not row([button, button, button])

Loskir · 2023-05-31T14:10:16Z

One use case that I'm thinking about rn is conditional buttons, e.g.

const keyboard = Keyboard.from([[button]])
if (userLoggedIn) {
  keyboard.append([[Keyboard.text('Profile')]])
}

append seems like a great option for this. But for this to work it should accept not only Keyboards, but also Button[][]
Or maybe you have other ideas? We are not limited by existing methods here

KnorpelSenf · 2023-05-31T14:10:20Z

Maybe add addRow as an alias for row?

What is the benefit of redundancy?

just like you added fromRows that is the same as from.

The use cases are shifting from chaining new Keyboard().text().row() to separate calls
const keyboard = Keyboard.from(initialData)
if (something) {
  keyboard.row(someConditionalData)
}
If this was new code, it would make a lot more sense to call it addRow instead of row. So why not add it to make such code more readable?

I don't see the pattern that you're referring to. That sounds like you also want to rename it to keyboard.addText("") and keyboard.addRequestLocation("") and so on?

KnorpelSenf · 2023-05-31T14:10:41Z

Forget about addRow, I just realized it accepts row(button, button, button) not row([button, button, button])

Oh okay

KnorpelSenf · 2023-05-31T14:11:41Z

One use case that I'm thinking about rn is conditional buttons, e.g.
const keyboard = Keyboard.from([[button]])
if (userLoggedIn) {
  keyboard.append([[Keyboard.text('Profile')]])
}
append seems like a great option for this. But for this to work it should accept not only Keyboards, but also Button[][] Or maybe you have other ideas? We are not limited by existing methods here

We could make it accept both Keyboard instances as well as anything else that Keyboard.from accepts. That would cover this use case, and it would be simple to implement. What do you think?

Loskir · 2023-05-31T14:12:51Z

Great idea! We could make Keyboard.from(new Keyboard()) a noop and then wrap anything passed in Keyboard.from

KnorpelSenf · 2023-05-31T14:17:22Z

Nice! Will do

Loskir · 2023-05-31T14:17:54Z

Speaking of reflow and fromColumn, how about this?

const keyboard = Keyboard.chunk([a, b, c], 1) // [[a], [b], [c]]

const keyboard = Keyboard.chunkReverse([a, b, c, d, e], 3) // [[a, b], [c, d, e]]

src/convenience/keyboard.ts

KnorpelSenf · 2023-05-31T14:24:09Z

Speaking of reflow and fromColumn, how about this?

Those are just array helper methods, right? So they don't really create keyboards

Loskir · 2023-05-31T14:25:58Z

Those are just array helper methods, right? So they don't really create keyboards

Maybe they do the transformation and then pass the result to Keyboard.from

codecov · 2023-05-31T14:27:35Z

Codecov Report

Patch coverage: 100.00% and project coverage change: +2.67 🎉

Comparison is base (0e7dbee) 38.67% compared to head (5f30bc9) 41.35%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #420      +/-   ##
==========================================
+ Coverage   38.67%   41.35%   +2.67%     
==========================================
  Files          16       16              
  Lines        4815     5035     +220     
  Branches      194      204      +10     
==========================================
+ Hits         1862     2082     +220     
  Misses       2951     2951              
  Partials        2        2

Impacted Files	Coverage Δ
src/composer.ts	`97.37% <ø> (ø)`
src/convenience/keyboard.ts	`100.00% <100.00%> (ø)`

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

KnorpelSenf · 2023-06-17T11:13:35Z

See EdJoPaTo/wikidata-misfit-bot#89

test/convenience/keyboard.test.ts

src/convenience/keyboard.ts

Co-authored-by: Dunkan <[email protected]>

wojpawlik · 2023-06-19T09:23:25Z

test/convenience/keyboard.test.ts

+ [["a", "c", "d"], ["b", "e"], ["f"]],
+ );
+ const keyboard = Keyboard.from([["a", "b", "c"], ["d", "e"], ["f"]]);
+ assertEquals(keyboard.toTransposed().toTransposed(), keyboard);


This is begging for a property-based test.

We don't do those yet. We can consider adding new test setups in future PQs.

test/convenience/keyboard.test.ts

deno.jsonc

src/convenience/keyboard.ts

The changes have been addressed

Loskir

LGTM 🎉

Co-authored-by: EdJoPaTo <[email protected]> Co-authored-by: Dunkan <[email protected]>

KnorpelSenf added 4 commits May 31, 2023 15:22

chore: improve test coverage generation reliability

9bad5c0

docs: fix wrong text flow

cea20a6

feat: keyboard button builders

e3d2d31

Closes #104.

test: cover keyboard button builders

4120e5c

KnorpelSenf requested a review from Loskir May 31, 2023 13:28

refactor: use for..of where possible

a6728dd

KnorpelSenf added 3 commits May 31, 2023 16:21

feat: support appending keyboard source data

709723f

refactor: remove lost import

c8f501f

chore: fix test coverage analysis

d4c7400

Loskir reviewed May 31, 2023

View reviewed changes

src/convenience/keyboard.ts Outdated Show resolved Hide resolved

fix: Keyboard.from(keyboard)

b75a2bc

test: improve coverage

f47631e

KnorpelSenf mentioned this pull request Jun 18, 2023

Roadmap 1.17: Reducing Verbosity #202

Closed

7 tasks

wojpawlik reviewed Jun 18, 2023

View reviewed changes

test/convenience/keyboard.test.ts Outdated Show resolved Hide resolved

perf(test): remove pointless clones from tests

a6f741b

KnorpelSenf requested a review from wojpawlik June 18, 2023 20:48

dcdunkan approved these changes Jun 18, 2023

View reviewed changes

src/convenience/keyboard.ts Outdated Show resolved Hide resolved

Update src/convenience/keyboard.ts

e8eb504

Co-authored-by: Dunkan <[email protected]>

wojpawlik reviewed Jun 19, 2023

View reviewed changes

EdJoPaTo reviewed Jun 19, 2023

View reviewed changes

src/convenience/keyboard.ts Outdated Show resolved Hide resolved

Loskir reviewed Jun 19, 2023

View reviewed changes

src/convenience/keyboard.ts Outdated Show resolved Hide resolved

KnorpelSenf added 2 commits June 19, 2023 20:06

feat: listen to feedback

d5792bf

Merge branch 'main' into keyboard-from

d767319

KnorpelSenf requested review from Loskir, wojpawlik, rojvv and dcdunkan June 19, 2023 18:06

test: adjust tests

36debfa

dcdunkan approved these changes Jun 20, 2023

View reviewed changes

EdJoPaTo reviewed Jun 21, 2023

View reviewed changes

src/convenience/keyboard.ts Show resolved Hide resolved

Loskir reviewed Jun 22, 2023

View reviewed changes

src/convenience/keyboard.ts Outdated Show resolved Hide resolved

Keyboard feedback

f848ea7

KnorpelSenf requested a review from Loskir June 25, 2023 11:16

KnorpelSenf added 2 commits June 25, 2023 13:17

Merge branch 'main' into keyboard-from

70166dc

docs: remove excessive @param docs

5f30bc9

Loskir approved these changes Jun 25, 2023

View reviewed changes

KnorpelSenf merged commit b0fcaa1 into main Jun 25, 2023
8 checks passed

KnorpelSenf deleted the keyboard-from branch June 25, 2023 22:25

KnorpelSenf added a commit to ssistoza/grammY that referenced this pull request Jun 25, 2023

feat: keyboard button builders (grammyjs#420)

385f572

Co-authored-by: EdJoPaTo <[email protected]> Co-authored-by: Dunkan <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: keyboard button builders #420

feat: keyboard button builders #420

KnorpelSenf commented May 31, 2023 •

edited

Loading

Loskir commented May 31, 2023

Loskir commented May 31, 2023

Loskir commented May 31, 2023

KnorpelSenf commented May 31, 2023

KnorpelSenf commented May 31, 2023

KnorpelSenf commented May 31, 2023

Loskir commented May 31, 2023

Loskir commented May 31, 2023

Loskir commented May 31, 2023

Loskir commented May 31, 2023

KnorpelSenf commented May 31, 2023

KnorpelSenf commented May 31, 2023

KnorpelSenf commented May 31, 2023

Loskir commented May 31, 2023

KnorpelSenf commented May 31, 2023

Loskir commented May 31, 2023

KnorpelSenf commented May 31, 2023

Loskir commented May 31, 2023

codecov bot commented May 31, 2023 •

edited

Loading

KnorpelSenf commented Jun 17, 2023

wojpawlik Jun 19, 2023

KnorpelSenf Jun 19, 2023

Loskir left a comment

feat: keyboard button builders #420

feat: keyboard button builders #420

Conversation

KnorpelSenf commented May 31, 2023 • edited Loading

Loskir commented May 31, 2023

Loskir commented May 31, 2023

Loskir commented May 31, 2023

KnorpelSenf commented May 31, 2023

KnorpelSenf commented May 31, 2023

KnorpelSenf commented May 31, 2023

Loskir commented May 31, 2023

Loskir commented May 31, 2023

Loskir commented May 31, 2023

Loskir commented May 31, 2023

KnorpelSenf commented May 31, 2023

KnorpelSenf commented May 31, 2023

KnorpelSenf commented May 31, 2023

Loskir commented May 31, 2023

KnorpelSenf commented May 31, 2023

Loskir commented May 31, 2023

KnorpelSenf commented May 31, 2023

Loskir commented May 31, 2023

codecov bot commented May 31, 2023 • edited Loading

Codecov Report

KnorpelSenf commented Jun 17, 2023

wojpawlik Jun 19, 2023

Choose a reason for hiding this comment

KnorpelSenf Jun 19, 2023

Choose a reason for hiding this comment

Loskir left a comment

Choose a reason for hiding this comment

KnorpelSenf commented May 31, 2023 •

edited

Loading

codecov bot commented May 31, 2023 •

edited

Loading