Add hidden legendPosition to Chart.
#7378
Conversation
tomalec
force-pushed
the
add/chart-hidden-legend
branch
from
29425c5
to
40d93fb
Compare
There was a problem hiding this comment.
@tomalec thanks for creating a pull request for this and for updating the story book as well.
I think this will be a great addition.
This tested well and the code looks good!
I am approving this, although for some bonus points it would be great to have some tests for this (I do realize we have zero tests for this component at the moment 🙈 so I am leaving it optional )
I'd love to, but had a hard time running the existing ones. Running all for the entire repo is too much for my machine ;). I tried |
Thanks for pointing that out, it indeed doesn't work very well after we restructured this. |
Thanks :) That works! Also, I cannot find any I tried /**
* @jest-environment jsdom
*/and naively mocking |
|
Using |
Oooh shoot that is not great, we do mock the I wonder if it an issue with the Clearly this requires some cleaning up. If this becomes to much of a hassle, you can merge it and I can create an issue for writing up some tests. |
The result is still the same:
|
|
I double-checked, that this is in fact complaining about the I thought updating (quite old I'm afraid I had to give up. I already spend 2 days trying to untangle the overload of dependencies, build configs, and legacy code. And I start to think it's not worth too much so much effort for Naturally, it would be worth updating all those legacy versions, but I start to think, it would be easier to burn |
|
The test I was trying to start with was:
|
|
For running a command in a single package, you can use the
|
Thanks for sharing the testing file, I had a look and tried it locally. You might not like the fix given it end up being pretty simple. I guess the example you saw used an old test renderer, we usually use the
/**
* @jest-environment jsdom
*/
/**
* External dependencies
*/
import { render } from '@testing-library/react';
import { createElement } from '@wordpress/element';
/**
* Internal dependencies
*/
import Chart from '../';
jest.mock('../d3chart', () => ({
D3Legend: jest.fn().mockReturnValue( '[D3Legend]' ),
}))
const data = [
{
date: '2018-05-30T00:00:00',
Hoodie: {
label: 'Hoodie',
value: 21599,
},
Sunglasses: {
label: 'Sunglasses',
value: 38537,
},
Cap: {
label: 'Cap',
value: 106010,
},
},
{
date: '2018-05-31T00:00:00',
Hoodie: {
label: 'Hoodie',
value: 14205,
},
Sunglasses: {
label: 'Sunglasses',
value: 24721,
},
Cap: {
label: 'Cap',
value: 70131,
},
},
];
describe( 'Chart', () => {
test( '<Chart legendPosition="hidden" /> should not render any legend', () => {
const { queryByText } = render( <Chart data={ data } legendPosition="hidden" /> );
expect( queryByText( '[D3Legend]' ) ).not.toBeInTheDocument();
} );
test( '<Chart legendPosition="bottom" /> should render the legend at the bottom', () => {
const { queryByText } = render( <Chart data={ data } legendPosition="bottom" /> );
expect( queryByText( '[D3Legend]' ) ).toBeInTheDocument();
} );
} ); |
@jeffstieler thanks 😄 I think the only down side with this approach is that it doesn't allow for using |
Sometimes, for example, when there is a single data set, there is no need for rendering the legend. It may even introduce more confusion than value. It seems interactive, but there is nothing you can do with it. Fixes: woocommerce/google-listings-and-ads#618
It was used but not explicitely stated.
louwie17
force-pushed
the
add/chart-hidden-legend
branch
from
27f05b1
to
617bfb7
Compare
from `react-test-renderer` to `{ render } from '@testing-library/react'`,
to avoid confision like #7378 (comment).
Update snapshots respectively.
|
FWIW, besides the |
|
@tomalec, here is a PR to update Jest to version 27. Until I started also updating several other dependencies some tests were failing for not obvious reasons, so hopefully that is the case here. When it gets merged, I'll give the test in this PR a spin. EDIT: I should refresh the page before commenting! I see the test has been fixed and merge 🎉 |
from `react-test-renderer` to `{ render } from '@testing-library/react'`,
to avoid confision like #7378 (comment).
Update snapshots respectively.
…n#7378) * Add `hidden` legendPosition to `Chart`. Sometimes, for example, when there is a single data set, there is no need for rendering the legend. It may even introduce more confusion than value. It seems interactive, but there is nothing you can do with it. Fixes: woocommerce/google-listings-and-ads#618 * Add `@storybook/addon-knobs` to devDependencies. It was used but not explicitely stated. * Add a changelog entry. * Add tests for legendPosition in Chart component Co-authored-by: Lourens Schep <lourensschep@gmail.com>
…n#7425) from `react-test-renderer` to `{ render } from '@testing-library/react'`, to avoid confision like woocommerce/woocommerce-admin#7378 (comment). Update snapshots respectively.
Add
hiddenlegendPosition toChart.Sometimes, for example, when there is a single data set, there is no need for rendering the legend. It may even introduce more confusion than value. It seems interactive, but there is nothing you can do with it.
Fixes:
legendPosition: 'hidden'warning on the report pages google-listings-and-ads#618Add @storybook/addon-knobs to devDependencies.
It was used but not explicitly stated.
Accessibility
prefers-reduced-motionScreenshots:
Detailed test instructions:
<Chart data={ data } legendPosition="hidden" >or
npm run storybooklegendPositionknob