Skip to content
This repository has been archived by the owner on Sep 10, 2022. It is now read-only.

add #renameProp, #renameProps and #withReducer examples #566

Closed
wants to merge 3 commits into from

Conversation

pokorson
Copy link
Contributor

@wuct another doc PR. I'm trying to introduce recompose to my team and they get a bit confuse about api description, what do you think about adding those examples?

Also it would be really nice to have some section about writing unit tests for components that uses recompose cause it gets a bit tricky. I'm still trying to wrap my head around how to write them. Should I create an issue to this discuss this further? Or is this something you discussed already?

@codecov-io
Copy link

codecov-io commented Nov 22, 2017

Codecov Report

Merging #566 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #566   +/-   ##
=======================================
  Coverage   88.37%   88.37%           
=======================================
  Files          49       49           
  Lines         370      370           
=======================================
  Hits          327      327           
  Misses         43       43

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f4745a0...27f3f03. Read the comment docs.

@istarkov
Copy link
Contributor

I have nothing to say about reducer, but renameProps seems like useless hoc this days, Isnt it? With modern es6 syntax it can be written much more easily. Ma be we start to deprecate such hocs?
@pokorson what are u think?

@pokorson
Copy link
Contributor Author

@istarkov I agree, personally I never used it. I'm not sure how you see it with ES6 syntax but personally I would just use #mapProps for this purpose. I guess reason for #renameProps is to slightly reduce code and better describe intention of operation you do. Not sure if it's worth to keep one more method in API for this.

and maybe do you have some comments on unit tests part? I saw you participated in few previous discussions about them?

@timkindberg
Copy link
Contributor

For unit testing here is what I’ve come up with: #407 (comment)

@pokorson
Copy link
Contributor Author

@istarkov should I remove renameProp and renameProps examples? Or can we just merge it?

@istarkov
Copy link
Contributor

Sorry, I don't know now, let's think more.
cc @wuct please suggest what to do

@pokorson
Copy link
Contributor Author

@istarkov sure, I also saw 2 issues regarding creating or updating documentation, will try to reach people responsible for them to get some insight of their work.

@wuct
Copy link
Contributor

wuct commented Dec 3, 2017

I've never used renameProp and renameProps either. I use mapProps instead as well.
I suggest we merge this PR with a deprecated warning in both the doc and the console.

@pokorson
Copy link
Contributor Author

pokorson commented Dec 3, 2017

@wuct are there any places in code I could look for such warning pattern?

@wuct
Copy link
Contributor

wuct commented Dec 6, 2017

@pokorson No, there are not. We've not deprecated anything before. I've changed my mind and just adding some note in the doc which telling people to use mapProps with ES6 syntax instead is good to me for now. We can write a migration guide in the Release page when we actually remove them.

docs/API.md Outdated
```
renameProp('oldName', 'newName')
->
mapProps(props => ({...props, newName: props.oldName}))
Copy link
Contributor

Choose a reason for hiding this comment

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

withProps not mapProps
withProps(p => ({ newProp: p.oldProp }))

@pokorson
Copy link
Contributor Author

pokorson commented Dec 6, 2017

@istarkov fixed

Copy link
Contributor

@wuct wuct left a comment

Choose a reason for hiding this comment

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

We should follow the style guide. Please add spaces after { and before }.

e.g.

withProps(props => ({ newName: props.oldName }))

@pokorson
Copy link
Contributor Author

@wuct fixed

@pokorson
Copy link
Contributor Author

hey @wuct I did git --amend and force push to fix your review so I guess it can't be reflected in diff right now :/ do you mind merging this one?

@pokorson pokorson closed this Dec 4, 2018
@pokorson pokorson deleted the doc_examples branch December 4, 2018 08:01
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.

None yet

5 participants