-
Notifications
You must be signed in to change notification settings - Fork 4
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 signatures #44
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👌 great job !
(just a little comment about a line in the README file, that is not updated in the PR)
README.md
Outdated
import factory from 'trampss-redux-factory' | ||
export default factory(/* middlewares */)('id')('api')('todos') | ||
import { map } from 'trampss-redux-factory' | ||
export default map({ key: 'id', path: 'api', name: 'todos' }) | ||
``` | ||
That's it, you exported a reducer function you can register thanks to combinerReducer in Redux. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"That's it, you just have to export the reducer function and you can register it through combinerReducer in Redux"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about That's it, you just exported the reducer function and now you can register it through combinerReducer in Redux.
?
The reducer function is already exported (this is the code above the sentence)
|
||
export default factory({ type: 'uniq', /* other options */ }) | ||
``` | ||
|
||
### state |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
space more after initialized: true,
|
||
// params checkers | ||
const isMiddleware = params => params.engine || params.pre || params.post | ||
const isOptions = params => (!!params.name) || (typeof params === 'string') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no pb on lint ? !!params.name
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why ? !!
is about converting a truthy value to true
} | ||
|
||
export const uniq = factory({ type: 'uniq' }) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
uniq is not connoted with the function uniq of lodash :/
uniq
=> single
?
map
=> multi
?
i think, the name of funciton dont match with the features.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree and I would prefer an issue to argue about it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
uniq
=> simple
?
map
=> complex
?
@bpetetot An idea ? :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#35
README : https://github.com/Trampss/trampss-redux-factory/blob/signature/README.md
TYPES : https://github.com/Trampss/trampss-redux-factory/blob/signature/TYPES.md