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

fix hoc displayName issue #4320

Merged
merged 1 commit into from Dec 24, 2018
Merged

fix hoc displayName issue #4320

merged 1 commit into from Dec 24, 2018

Conversation

@rkurbatov
Copy link
Contributor

rkurbatov commented Dec 20, 2018

Our tests rely on component names and with special hoc additon that was broken. Nested components were named as Component. That fix restores original naming.

@codecov

This comment has been minimized.

Copy link

codecov bot commented Dec 20, 2018

Codecov Report

Merging #4320 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@          Coverage Diff           @@
##           master   #4320   +/-   ##
======================================
  Coverage     100%    100%           
======================================
  Files          74      74           
  Lines        1676    1678    +2     
======================================
+ Hits         1676    1678    +2
Impacted Files Coverage Δ
src/ReduxFormContext.js 100% <100%> (ø) ⬆️

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 e7aa280...6c616d9. Read the comment docs.

2 similar comments
@codecov

This comment has been minimized.

Copy link

codecov bot commented Dec 20, 2018

Codecov Report

Merging #4320 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@          Coverage Diff           @@
##           master   #4320   +/-   ##
======================================
  Coverage     100%    100%           
======================================
  Files          74      74           
  Lines        1676    1678    +2     
======================================
+ Hits         1676    1678    +2
Impacted Files Coverage Δ
src/ReduxFormContext.js 100% <100%> (ø) ⬆️

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 e7aa280...6c616d9. Read the comment docs.

@codecov

This comment has been minimized.

Copy link

codecov bot commented Dec 20, 2018

Codecov Report

Merging #4320 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@          Coverage Diff           @@
##           master   #4320   +/-   ##
======================================
  Coverage     100%    100%           
======================================
  Files          74      74           
  Lines        1676    1678    +2     
======================================
+ Hits         1676    1678    +2
Impacted Files Coverage Δ
src/ReduxFormContext.js 100% <100%> (ø) ⬆️

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 e7aa280...6c616d9. Read the comment docs.

Copy link
Collaborator

gustavohenke left a comment

LGTM -- however, please note that this is a breaking change, and if you updated from v7 to v8, it is expected that some things would break.
Might be worth adding some snapshot-like tests.

WDYT @erikras?

@rkurbatov

This comment has been minimized.

Copy link
Contributor Author

rkurbatov commented Dec 21, 2018

It's fine to have breaking changes but it's not fine to have Connect(Component) instead of Connect(ReduxForm) or ForwardRef instead of expected Field - that's pretty... ehm... non-informative.

There are two common approaches for any wrapper component naming:

  1. Pass the name of the wrapped component transparently (as here)
  2. Pass the name of the wrapped component with wrapper name surrounded (like withReduxForm(WrappedComponent) or withForwardRef(Field) like standard react-redux connector does.

I'm ok with both approaches even if the second one requires rewriting the tests. But as to mee it needs to be handled somehow.

@erikras

This comment has been minimized.

Copy link
Member

erikras commented Dec 24, 2018

This seems fine to me.

@erikras erikras merged commit 31048ad into redux-form:master Dec 24, 2018
4 checks passed
4 checks passed
codeclimate All good!
Details
codecov/patch 100% of diff hit (target 100%)
Details
codecov/project 100% (+0%) compared to e7aa280
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@erikras

This comment has been minimized.

Copy link
Member

erikras commented Dec 24, 2018

Published in v8.1.0.

@rkurbatov

This comment has been minimized.

Copy link
Contributor Author

rkurbatov commented Dec 31, 2018

Thanks! Happy New Year!

@lock

This comment has been minimized.

Copy link

lock bot commented Jan 30, 2019

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@lock lock bot locked as resolved and limited conversation to collaborators Jan 30, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
3 participants
You can’t perform that action at this time.