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

feat(sfint-3272): Add ToggleActionButton component #76

Merged
merged 4 commits into from
Jun 19, 2020

Conversation

lbergeron
Copy link
Collaborator

This PR adds the ToggleActionButton component. As its name states, it simply is an ActionButton featuring a toggle state. In addition, to keep things simple, this button supports only having an icon and tooltip. A different icon and tooltip can be displayed when the button is activated.

Using the options, the caller might attach handlers when the button is either clicked, activated, or deactivated.

@nathanlb
Copy link
Contributor

Looks great! Just out of curiosity had you considered extending the ActionButton as an alternative? If yes then would you mind explaining why you went this route instead?

@lbergeron
Copy link
Collaborator Author

Looks great! Just out of curiosity had you considered extending the ActionButton as an alternative? If yes then would you mind explaining why you went this route instead?

Yes, and yes. By extending ActionButton, ToggleActionButton would also have to support all possible combinations of icon/title. Composition allows to implement only what is needed right now. If we ever need to have toggle buttons with text, it would be possible to add it.

@nathanlb
Copy link
Contributor

Looks great! Just out of curiosity had you considered extending the ActionButton as an alternative? If yes then would you mind explaining why you went this route instead?

Yes, and yes. By extending ActionButton, ToggleActionButton would also have to support all possible combinations of icon/title. Composition allows to implement only what is needed right now. If we ever need to have toggle buttons with text, it would be possible to add it.

All right thanks. That was my suspicion. Well, GG! 🏆

Copy link
Collaborator

@louis-bompart louis-bompart left a comment

Choose a reason for hiding this comment

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

LGTM overall,
Some changes maybe on the variable names (I'm being picky tho).
Intrigued by the test with the spy.

src/components/ActionButton/ToggleActionButton.ts Outdated Show resolved Hide resolved
src/components/ActionButton/ToggleActionButton.ts Outdated Show resolved Hide resolved
tests/components/ActionButton/ToggleActionButton.spec.ts Outdated Show resolved Hide resolved
tests/components/ActionButton/ToggleActionButton.spec.ts Outdated Show resolved Hide resolved
@mikegron
Copy link
Collaborator

mikegron commented Jun 18, 2020

Très nice! J'aime bien ces petits components la :P
Question comme ca, est-ce qu'ont pourrais utiliser ce component la dans le component de AttachResult ou même le remplacer complètement, vue que lui aussi a un state de toggle avec une checkbox, mais se trouve dans le ResultActionMenu ?
Le seul truc additionnel dans AttachResult c'est l'option pour aller chercher le state initial et les custom Events de attach/detach je crois bien.

@lbergeron
Copy link
Collaborator Author

Très nice! J'aime bien ces petits components la :P
Question comme ca, est-ce qu'ont pourrais utiliser ce component la dans le component de AttachResult ou même le remplacer complètement, vue que lui aussi a un state de toggle avec une checkbox, mais se trouve dans le ResultActionMenu ?
Le seul truc additionnel dans AttachResult c'est l'option pour aller chercher le state initial et les custom Events de attach/detach je crois bien.

C'est une bonne question, je n'y avais pas songé, mais c'est vrai que les boutons ont un comportement qui se ressemble beaucoup. Je ne sais pas si le changement pourrait se faire sans briser la compatibilité par contre.

@louis-bompart louis-bompart self-requested a review June 19, 2020 13:09
Copy link
Collaborator

@louis-bompart louis-bompart left a comment

Choose a reason for hiding this comment

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

LGTM! :shipit:

@lbergeron lbergeron merged commit 09fce17 into coveo:master Jun 19, 2020
@lbergeron lbergeron deleted the feature/SFINT-3272 branch June 19, 2020 13:43
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.

None yet

5 participants