search

wireapp / generic-message-proto

Protocol definition for generic messages.
https://www.npmjs.com/package/@wireapp/protocol-messaging
GNU General Public License v3.0
33 stars 25 forks source link

Rename Button to MultipleChoiceButton #50

Closed typfel closed 4 years ago

typfel commented 4 years ago

I propose to rename the message Button to message MultipleChoiceButton. This will make it more clear how the buttons in the CompositeMessage should behave and allows us to add other buttons types in the future.

David-Henner commented 4 years ago

Wouldn't it be confusing? To me naming it MultipleChoiceButton implies that it's a single button providing multiple choices

marcoconti83 commented 4 years ago

What about ChoiceButton, OptionButton?

typfel commented 4 years ago

I was looking at it as one a button/entry in multiple choice component and therefore naming it MultipleChoiceButton.

I went with multiple choice since that's a very well known concept https://en.wikipedia.org/wiki/Multiple_choice

OptionButton is also good IMO, it doesn't hint that only one option can be selected though but on the other and it's more clear that it's only one of many options.

dkovacevic commented 4 years ago

I really don't see how this button is a MultiChoice button. You have a choice to click on it or not. If those 2 choices then yes.. it's a multichoice, sure. It's the most generic button I can think of. Like any other button: something happens when you click on it...

typfel commented 4 years ago

I really don't see how this button is a MultiChoice button. You have a choice to click on it or not. If those 2 choices then yes.. it's a multichoice, sure. It's the most generic button I can think of. Like any other button: something happens when you click on it...

I agree. As it is designed here in the protobuf it's a very generic button, but that's not the semantic that's expected to be implemented by the clients. What's described in the the technical specification looks like multiple choice to me: https://github.com/wearezeta/documentation/blob/master/topics/messages/use%20cases/003-buttons.md

gizemb commented 4 years ago

I think renaming this to MultiChoiceButton implies that there can also be a SingleChoiceButton. Is that something we want to support in the future?

typfel commented 4 years ago

I think renaming this to MultiChoiceButton implies that there can also be a SingleChoiceButton. Is that something we want to support in the future?

I realise now that MultipleChoiceButton is confusing. As I understood it we are implementing a logic where you need to pick one out of many options, and that's what I meant by MultiChoiceButton.

Maybe OptionButton is less confusing. I think it will always be a little bit confusing when we are dealing with buttons which are implicitly linked to together by just being in the same message.

marcoconti83 commented 4 years ago

After a protracted in-person discussion, we came to the conclusion that we all disagree on a name :) the one thing we agree on is that if we need to expand this in the future, we can add a type property to the Button, to cover future cases.

Therefore we take the decision to stick with Button now even if it's not the best, to be able to move on with the implementation.