Skip to content
This repository was archived by the owner on Apr 28, 2022. It is now read-only.

Dev switch option #47

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

tianzhich
Copy link

@tianzhich tianzhich commented Nov 21, 2019

  1. Mark babel-loader as dependency for non-babel users.
  2. Use babel-loader@7.1.5 to avoid some errors.
  3. Add canSwitch option.

closes #46

@Kikobeats
Copy link
Owner

why babel-loader as dependency is necessary? 🤔

@tianzhich
Copy link
Author

According to storybookjs/storybook#4116, babel-loader is peerDependencies of @storybook/react.
So project using storybook should mark it as dependency.😁

@Kikobeats
Copy link
Owner

What do you think about to take this an opportunity to expose an inversion of control?
https://kentcdodds.com/blog/inversion-of-control

Thinking this is a very specific case and we are just adding much code there

@tianzhich
Copy link
Author

tianzhich commented Nov 25, 2019

Do you have any ideas here?

Cause react-clap-button is not a fully-controlled component, the count and countTotal change depends on internal state.
I think it could be fully controlled if we passed the count, or countTotal similarly. And the count could be defaultCount accordingly. So increase shows +1 and decrease shows -1.

Then users can implement switch effect if they want.

const { count, countTotal, isClicked, isHover } = this.state
const { iconComponent: ClapIcon } = this.props
const { iconComponent: ClapIcon, canSwitch } = this.props
const clapCountText = canSwitch ? `${isClicked ? '+' : '-'}1` : `+${count}`
Copy link

@auspham auspham Feb 9, 2020

Choose a reason for hiding this comment

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

please change the hard-coded count +1.

What if there is the case where synchronization is used to make a real-time update for the clapCountText?

If that so we have to compare the current count with the previous count, cannot just depend on isClicked

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

Successfully merging this pull request may close these issues.

Add option for switch
3 participants