Skip to content
This repository has been archived by the owner on Mar 4, 2020. It is now read-only.

fix(Provider): should not use felaRenderer explicitly #1842

Merged
merged 14 commits into from
Aug 26, 2019

Conversation

lucivpav
Copy link
Contributor

Fixes #1814

@vercel vercel bot temporarily deployed to staging August 23, 2019 10:08 Inactive
@codecov
Copy link

codecov bot commented Aug 23, 2019

Codecov Report

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

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1842   +/-   ##
=======================================
  Coverage   69.64%   69.64%           
=======================================
  Files         874      874           
  Lines        7610     7610           
  Branches     2215     2242   +27     
=======================================
  Hits         5300     5300           
  Misses       2302     2302           
  Partials        8        8
Impacted Files Coverage Δ
...ackages/react/src/components/Provider/Provider.tsx 93.47% <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 3971070...e2e243a. Read the comment docs.

Copy link
Member

@layershifter layershifter left a comment

Choose a reason for hiding this comment

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

Let's add some tests for it

@layershifter layershifter added the 🧰 fix Introduces fix for broken behavior. label Aug 23, 2019
@vercel vercel bot temporarily deployed to staging August 26, 2019 10:22 Inactive
{
files: '**/*.{ts,tsx}',
rules: {
'no-dupe-class-members': 'off',
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Otherwise we get false positive in RendererMock.tsx, see typescript-eslint/typescript-eslint#291

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No longer needed at the moment, but I think it will be useful to keep this change, as described in the link above

Copy link
Member

Choose a reason for hiding this comment

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

Yep, let's keep it

@lucivpav
Copy link
Contributor Author

Let's add some tests for it

@layershifter I have added tests. But I feel its a bit ugly to add RendererMock.tsx file. Can you point me to the right direction?

@vercel vercel bot temporarily deployed to staging August 26, 2019 10:25 Inactive
}

const renderFontCalled = jest.fn()
const rendererMock = new RendererMock(renderFontCalled, jest.fn())
Copy link
Member

Choose a reason for hiding this comment

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

const theme: ThemeInput = {
fontFaces: teams.fontFaces,
}
const renderer = createRenderer()
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 cannot use felaRenderer, because the test would pass even without this fix.

describe('calls provided renderer', () => {
test('calls renderFont', () => {
const theme: ThemeInput = {
fontFaces: teams.fontFaces,
Copy link
Member

Choose a reason for hiding this comment

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

Can we use some dumb definition instead?

@lucivpav lucivpav merged commit f46279c into master Aug 26, 2019
@delete-merged-branch delete-merged-branch bot deleted the fix/provider-renderer branch August 26, 2019 15:37
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
🧰 fix Introduces fix for broken behavior. 🚀 ready for review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Provider doesn't always use props.renderer
2 participants