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

Added onInjectedCallback #16

Conversation

drew-diamantoukos
Copy link
Contributor

The primary new feature is adding a onInjectedCallback prop. The API matches the latest ReactSVG API for the onInjected prop :

Optional Function to call after the SVG is injected. If an injection error occurs, this function receives an Error object as the first parameter. Otherwise, the first parameter is null and the second parameter is the injected SVG DOM element. Defaults to () => {}.

  • Bumped version to 2.1.0 since only a new prop was added.
  • Updated ReactSVG library to major version 7 to leverage their async callback capabilities which have been added since version 4.
  • Updated testing libraries (enzyme, jest, et al) in order for AnataomogramSvg component to be snapshotted as HTML instead of an Enzyme object - also reduced test boilerplate by adding jest-enzyme and the corresponding testEnvironment.
  • Touched up Markdown to annotate language for code snippets / added to spacing for consistency.
  • Added my name to the contributors' list 😂

@coveralls
Copy link

coveralls commented Jan 14, 2019

Pull Request Test Coverage Report for Build 97

  • 4 of 5 (80.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+3.9%) to 67.722%

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/AnatomogramSvg.js 4 5 80.0%
Totals Coverage Status
Change from base Build 94: 3.9%
Covered Lines: 70
Relevant Lines: 101

💛 - Coveralls

@alfonsomunozpomer
Copy link
Member

alfonsomunozpomer commented Jan 15, 2019

Many thanks for the PR!

Just a couple of observations:
• In AnatomogramSvg.test.js, the test should call onInjectedCallback when the svg is injected isn’t actually verifying that the callback function has been called.
• In terms of consistency, we only use test instead of it and we don’t use semicolons in tests. Also, if you name the prop onInjected, I think it’ll be also consistent with other props and it’ll read more natural when you use it with e.g. Eznyme’s simulate.

Could you please fix the above? Other than that it looks good. Thank you for adding jest-environment-enzyme to clear boilerplate stuff.

… removed semicolons, renamed to , added a comment explaining the done callback for jest.
@drew-diamantoukos
Copy link
Contributor Author

Ah, thanks for the feedback! I definitely approve of consistency, apologies for missing some of them -the latest should reflect all the suggestions.

As for the onInjected test, that is actually testing to ensure it is being called. Based on the jest docs for async callbacks, the test will pass if done (and thus, onInjected) is called and fail otherwise.

@drew-diamantoukos
Copy link
Contributor Author

Hey @alfonsomunozpomer, just sending a quick ping in case you missed the pushed changes to the PR!

Copy link
Member

@alfonsomunozpomer alfonsomunozpomer left a comment

Choose a reason for hiding this comment

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

All good, sorry it took a while to get this merged.

@alfonsomunozpomer alfonsomunozpomer merged commit 5a4d21e into ebi-gene-expression-group:master Mar 1, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants