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

Framework: Replace flowRight occurences with compose for HOCs #3907

Merged
merged 2 commits into from Dec 12, 2017

Conversation

Projects
None yet
3 participants
@gziolo
Member

gziolo commented Dec 11, 2017

Description

In #3493 new utility method compose was introduced to be used with Higher-Order Components. This PR refactors all occurrences of flowRight around HOCs to be replaced with compose.

How Has This Been Tested?

Manually. All tests should still pass, linter should be happy and editor should work without errors.

Types of changes

Refactoring only. No changes in the existing functionalities.

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows has proper inline documentation.

@gziolo gziolo added the Framework label Dec 11, 2017

@gziolo gziolo self-assigned this Dec 11, 2017

@gziolo gziolo requested review from youknowriad and aduth Dec 11, 2017

@youknowriad

This comment has been minimized.

Show comment
Hide comment
@youknowriad

youknowriad Dec 11, 2017

Contributor

What about flow usage, I think we have some as well :)

Contributor

youknowriad commented Dec 11, 2017

What about flow usage, I think we have some as well :)

@gziolo

This comment has been minimized.

Show comment
Hide comment
@gziolo

gziolo Dec 11, 2017

Member

So I have just learned that flow and flowRight were used as if they were the same. That makes replacing flow easier and validates the need of compose helper method :)

Member

gziolo commented Dec 11, 2017

So I have just learned that flow and flowRight were used as if they were the same. That makes replacing flow easier and validates the need of compose helper method :)

@aduth

This comment has been minimized.

Show comment
Hide comment
@aduth

aduth Dec 11, 2017

Member

So I have just learned that flow and flowRight were used as if they were the same.

That's not entirely true; in many cases we're assuming props to be passed from a previous HoC in the array, which works with flowRight, but not flow. See also: #2500 (comment)

Member

aduth commented Dec 11, 2017

So I have just learned that flow and flowRight were used as if they were the same.

That's not entirely true; in many cases we're assuming props to be passed from a previous HoC in the array, which works with flowRight, but not flow. See also: #2500 (comment)

@gziolo

This comment has been minimized.

Show comment
Hide comment
@gziolo

gziolo Dec 11, 2017

Member

@aduth if you will take a closer look, you will notice that connect almost always goes first and it doesn't matter whether it's used with flow or flowRight. This is where my comment comes from. If you are aware of any place where I should update the order, let me know.

Member

gziolo commented Dec 11, 2017

@aduth if you will take a closer look, you will notice that connect almost always goes first and it doesn't matter whether it's used with flow or flowRight. This is where my comment comes from. If you are aware of any place where I should update the order, let me know.

@aduth

aduth approved these changes Dec 11, 2017

@gziolo gziolo merged commit dfa9e5e into master Dec 12, 2017

3 checks passed

codecov/project 38.21% (-0.01%) compared to ad5db70
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@gziolo gziolo deleted the update/flow-right-compose branch Dec 12, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment