Reference react-vis properly from showcase. #1326
Conversation
Showcase was using 'index' when importing 'react-vis'; This causes issues when trying to migrate to babel 7.
packages/react-vis/package.json
Outdated
| ], | ||
| "alias": { | ||
| "react-vis/*": "src/*" | ||
| } |
There was a problem hiding this comment.
This seems incorrect? shouldn't this be in showcase?
There was a problem hiding this comment.
Theres a whole bunch of indirection happening with the references between these projects. I'm trying to narrow it down.
Of course, these tests all work on my machine :P
| @@ -162,9 +161,8 @@ test('testing flexible charts', t => { | |||
| test('Render two stacked bar series with a non-stacked line series chart', t => { | |||
| const $ = mount(<MixedStackedChart />); | |||
|
|
|||
| const renderedBarsWrapper = $.find(BarSeries); | |||
| const renderedLineWrapper = $.find(LineSeries); | |||
There was a problem hiding this comment.
Pretty sure this was correct before. Why do we need to change this?
There was a problem hiding this comment.
The BarSeries that the showcase is referencing, is different than the BarSeries in the test.
There was a problem hiding this comment.
fixed the imports and I was able to change this back so that its properly referencing the same instance of the components.
| @@ -19,7 +19,7 @@ | |||
| // THE SOFTWARE. | |||
|
|
|||
| import React from 'react'; | |||
| import {AreaSeries, HorizontalBarSeries, XAxis, XYPlot, YAxis} from 'index'; | |||
| import {AreaSeries, HorizontalBarSeries, XAxis, XYPlot, YAxis}from 'react-vis'; | |||
There was a problem hiding this comment.
for some reason prettier doesn't seem to be working. I can look into this later.
| @@ -20,7 +20,7 @@ | |||
|
|
|||
| import React from 'react'; | |||
|
|
|||
| import DiscreteColorLegend from 'legends/discrete-color-legend'; | |||
| import DiscreteColorLegend from 'react-vis/legends/discrete-color-legend'; | |||
There was a problem hiding this comment.
We might want to check whether react-vis exports components in this way, since these showcase files can be used to demonstrate code and be easily-copy-pastable. But that can be a future task. This change seems to be in the right direction.
There was a problem hiding this comment.
This is the way that react-vis exports items.
Although, we should eventually change these imports only use the root react-vis.
This allows us to easily curate our public api.
In general, you shouldn't really import more than one level into a library as it tends to be internal. baseui just recently added lint warnings if you try to go more than 2 levels deep in your imports.
There was a problem hiding this comment.
actually... I need to test this..
| "." | ||
| ], | ||
| "alias": { | ||
| "react-vis/*": "../react-vis/src/*", |
There was a problem hiding this comment.
Interesting that we need this statement. Makes sense.
Do we need to remove some duplication from the storybook webpack config?
There was a problem hiding this comment.
We need this so that the tests will pick up the same instances of the components as the tests.
Eventually we will need the tests to use their own plots.
The way things are now, a simple change to the showcase can break the tests, which definitely isn't ideal.
Showcase was using 'index' when importing 'react-vis';
This causes issues when trying to migrate to babel 7.