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

Factor out common code for markup in React plugins #4171

Merged
merged 6 commits into from
Aug 1, 2017

Conversation

pedrottimark
Copy link
Contributor

Summary

Enforce consistency, reduce code. Like the TV advert: looks pretty, less filling :)

We will probably be able to factor out common code in HTMLElement too.

Analogy to react-redux as you review this diff:

  • React plugins like container components?
  • markup functions like presentation components?

Test plan

Added 2 tests to prevent regression related to test object for subset match, especially because of the incorrect Flow error.

@thymikee
Copy link
Collaborator

thymikee commented Aug 1, 2017

Needs prettier!

config: Config,
indentation: string,
): string => {
const tag = config.colors.tag;
Copy link
Collaborator

Choose a reason for hiding this comment

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

colorTag maybe?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I replaced it with tagColor but am happy to change to colorTag if that is clearer

getChildren(item, children);
});
} else if (arg != null && arg !== false) {
children.push(arg);
Copy link
Collaborator

Choose a reason for hiding this comment

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

I like the simplification of this callback 👍

Copy link
Collaborator

@thymikee thymikee left a comment

Choose a reason for hiding this comment

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

LGTM

@codecov-io
Copy link

codecov-io commented Aug 1, 2017

Codecov Report

Merging #4171 into master will decrease coverage by 0.14%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master   #4171      +/-   ##
=========================================
- Coverage   60.55%   60.4%   -0.15%     
=========================================
  Files         196     197       +1     
  Lines        6776    6751      -25     
  Branches        6       6              
=========================================
- Hits         4103    4078      -25     
  Misses       2670    2670              
  Partials        3       3
Impacted Files Coverage Δ
.../pretty-format/src/plugins/react_test_component.js 100% <100%> (ø) ⬆️
packages/pretty-format/src/plugins/lib/markup.js 100% <100%> (ø)
...ackages/pretty-format/src/plugins/react_element.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 9665de2...9c5920d. Read the comment docs.

@pedrottimark
Copy link
Contributor Author

This race was too close to call, which is as you might expect:

  • In each row, the ranges of 3 factored and 3 unfactored runs overlapped in one value.
  • But I don’t know why: twice as much variation among factored runs compared to unfactored.
scenario plugins factored unfactored ratio
react_test_component 9 729251 696248 1.047
react_element 9 806585 822724 0.980

@cpojer cpojer merged commit f47d7de into jestjs:master Aug 1, 2017
@cpojer
Copy link
Member

cpojer commented Aug 1, 2017

Super sweet cleanup.

@pedrottimark pedrottimark deleted the plugins-lib-markup branch August 1, 2017 21:20
tushardhole pushed a commit to tushardhole/jest that referenced this pull request Aug 21, 2017
* Factor out common code for markup in React plugins

* Add mistakenly deleted export

* Put getType after getChildren to improve diff

* Make the 2 new tests parallel

* Fix pretty lint error

* Replace tag with tagColor
@github-actions
Copy link

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.
Please note this issue tracker is not a help forum. We recommend using StackOverflow or our discord channel for questions.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 13, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants