Closed m4dz closed 8 years ago
Well, this PR is too big for github so review out of context
require(linkedstate-mixin)
s to the top of filetoArray
+ key
instead of createFragment
when it works. Alternatively, we could make an helper function to optimise the use case fn(Immutable.Map, (key, value) -> ReactElement) -> Array(ReactElement)
basic_components
not sure how to do it better, but i dislike the inversion of createFactory position. Maybe we could have an helper createFactories
?alert
module.exports = createFactory findDOMNode
vs ref
, pick one and stick to it ( :+1: findDOMNode)date_range_picker
/ talker
/ plugin_utils
file_picker
createFragment
required & not used client/app/components/mailbox_picker.coffee
manifest.json
in vendor
?cached-transforms
by reselect
or alternatives ( @misstick had a good one), could wait the store-to-getters refactoringAnd then, you will need to rebase against @misstick recent changes :-/
Oups, forgot to mention the main point of this review : This is awesome !
This is awesome !
:kissing_cat:
About basic_components
, what if we export the basic components as a factory:
module.exports = createFactories (components...) ->
res = {}
for component in components
if registry[name]
res[component] = React.createFactory registry[name]
return res
and then call requires like:
{FieldSet, FormButtons, FormButton} = require('./basic_components')('FieldSet', 'FormButtons', 'FormButton')
findDOMNode vs ref, pick one and stick to it ( :+1: findDOMNode)
From https://facebook.github.io/react/docs/top-level-api.html#reactdom.finddomnode:
In most cases, you can attach a ref to the DOM node and avoid using findDOMNode at all.
So, I suggest we stick to refs
instead of findDOMNode
except if we can't.
weird filemode change on all assets
does this really matters?
why manifest.json in vendor ?
I haven't any manifest.json
file in vendor
locally. Are you sure about this file?
server/config.coffee typo locales (langs) -> locals (local variables)
I can't find it… Can you higlight it in my fork's branch? Thanks!
@aenario I left your ideas for improvements outside of this PR, but I totally agree to all of them :smile:
Thanks @m4dz for this PR :+1:
I encountered some errors, I paste it on https://gist.github.com/poupotte/0629e4ffd06762024ae7 :
I continue my tests ...
For the typo : it will not cause a bug, its just the wrong word for what is being passed (should be locals
)
https://github.com/m4dz/cozy-emails/blob/d82f120cd9073ee805e7782500b8de07d839ffe7/server/config.coffee#L20-L21
:+1: for using ref everywhere.
filemode change, doesn't really matter.
Regarding vendor, it's the whole assets folder https://github.com/m4dz/cozy-emails/tree/d82f120cd9073ee805e7782500b8de07d839ffe7/client/vendor/assets which I am not sure why you put them in vendor instead of app.
For basic_component
, what about
{FieldSet, FormButtons, FormButton} = require('./basic_components').Factories
It still is explicit on require side that we are taking the factories and not the classes and is simpler, and we just have to add
export.Factories[key] = createFactory klass for key, klass of registry
in basic components
@aenario, @poupotte: your reviews are theorically fixed. Can you pass some tests to see if everythings is OK? Thanks!
Can we merge that one?
@m4dz Can you rebase it ?
Done for rebase
Thank you all for that. It's a lot of work. It should make the development much cleaner. I can't wait to see tests on the client side!
This PR introduces deep changes:
THIS PR NEEDS TO BE TESTED DEEPLY BEFORE MERGING