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

Change the logic of creating the Keyboard and Keyboard Buttons #1344

Closed
curbmoon opened this issue Jul 18, 2022 · 5 comments · Fixed by #1395
Closed

Change the logic of creating the Keyboard and Keyboard Buttons #1344

curbmoon opened this issue Jul 18, 2022 · 5 comments · Fixed by #1395

Comments

@curbmoon
Copy link

curbmoon commented Jul 18, 2022

It is very inconvenient that it is impossible to create an inline button with empty parameters in order to operate on it in the future and you have to manually translate the callback_data array every time, so you have to do this:

$keyboardInline = new \Longman\TelegramBot\Entities\InlineKeyboard([]);  
 $baseInlineKeyboardButton_1 =  new \Longman\TelegramBot\Entities\InlineKeyboardButton(['text' => 'Text.',  
 'callback_data' =>json_encode([  
       'name' => 'test_button_1',  
       'show' => true  
    ])]);  
 $baseInlineKeyboardButton_2 =  new \Longman\TelegramBot\Entities\InlineKeyboardButton(['text' => 'Text', 'callback_data' =>json_encode([  
       'name' => 'test_button_2',  
       'show' => true  
    ])]);  
$keyboardInline->addRow(  
 $baseInlineKeyboardButton_1,  
 $baseInlineKeyboardButton_2  
);  

I think it would be more comfortable and convenient:

$keyboardInline = new \Longman\TelegramBot\Entities\InlineKeyboard([]);  

$baseInlineKeyboardButton =  new \Longman\TelegramBot\Entities\InlineKeyboardButton();  
$keyboardInline = new \Longman\TelegramBot\Entities\InlineKeyboard([]);  
$keyboardInline->addRow(  
$baseInlineKeyboardButton->setText('Тестовая кнопка #1')->setCallbackData(([  
       'name' => 'test_button_1',  
       'show' => true  
]),  
$baseInlineKeyboardButton->setText('Тестовая кнопка #2')->setCallbackData(([  
       'name' => 'test_button_2',  
       'show' => true  
])  
);  

without the need to create a new button object each time, specifying all parameters at once and without manual json_encode

@noplanman
Copy link
Member

noplanman commented Jul 18, 2022

EDIT: Ignore my example below, the problem here is that an inline keyboard object can't be created with just a text element, as the validation that expects the callback_data (or other) property happens in the constructor.

i.e. this fails: new InlineKeyboardButton('Some text');

@TiiFuchs Maybe we should skip this validation entirely and let the Telegram API complain about it?


I understand what you mean, but don't see such a big difference between the two snippets, as you could inline the new button objects too, which would look pretty much the same:

$keyboardInline = new InlineKeyboard([]);
$keyboardInline->addRow(
    (new InlineKeyboardButton('Тестовая кнопка #1'))->setCallbackData(json_encode([
        'name' => 'test_button_1',
        'show' => true,
    ])),
    (new InlineKeyboardButton('Тестовая кнопка #2'))->setCallbackData(json_encode([
        'name' => 'test_button_2',
        'show' => true,
    ]))
);

Also, in your example you're overwriting the $baseInlineKeyboardButton for the second button, so both buttons will have the same values.
Unless there was an actual method that ends the chaining and outputs the data of that button.

@TiiFuchs What do you think?

@TiiFuchs
Copy link
Member

@noplanman I agree. I think we shouldn’t do any validation within this class and keep that to the API servers. So we should remove the validation.

@TiiFuchs
Copy link
Member

Someone willing to remove it? ;D

@noplanman
Copy link
Member

@TiiFuchs Would make sense to kill it before the next release, what do you think?

@TiiFuchs
Copy link
Member

Now is as good as anytime.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants