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

chore: add TodoMVC as example #103

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

chore: add TodoMVC as example #103

wants to merge 1 commit into from

Conversation

Saviio
Copy link
Contributor

@Saviio Saviio commented Jan 21, 2019

这里还有一个分支:
https://github.com/ReactiveDB/core/tree/chore/example-v2

我已经分不清哪个 branch 更值得推荐了...

我的主要问题在于,怎么写 dispatcher,视图组件的划分逻辑,要不要用 context

P.S.
PR 的这个版本是使用了 context 的,建议 clone 下来 review

@Saviio Saviio requested a review from Brooooooklyn January 21, 2019 05:53
@coveralls
Copy link

coveralls commented Jan 21, 2019

Coverage Status

Coverage remained the same at 96.733% when pulling 7d959fc on chore/example into 2311825 on master.

@Saviio Saviio force-pushed the chore/example branch 3 times, most recently from 0b0a6e0 to 4bb86ca Compare January 21, 2019 06:05
import { source, TodoContext } from '../controller'
import { useObservable } from '../hooks'

export const Filter = React.memo(() => {

Choose a reason for hiding this comment

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

will there have any problem using React.memp with empty props?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i think this should be ok, just like

shouldComponentUpdate() {
  return false
}

@mr-pinzhang
Copy link

can't tell differences between both, imo, the context only injects code inside component, which make it as a more coupled component. besides, no differences to me.

@mr-pinzhang
Copy link

@Saviio really hard to cr, could you post key changes as a snippet image?

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.

3 participants