Country selector component integrated with country performance graphs #17
Conversation
…. as previous was not recognizable for jest
…er country is selected.
| } | ||
| this.updateValue1 = this.updateValue1.bind(this) | ||
| this.updateValue2 = this.updateValue2.bind(this) | ||
| this.updateValue3 = this.updateValue3.bind(this) | ||
| } |
There was a problem hiding this comment.
I Really doubt having separate function for each selector handling dropdown changes is the right approach. Especially when there will be multiple graphs.
We need to explore more how this may be done.
There was a problem hiding this comment.
@zelima when there will be multiple graphs they will be instantiated from different containers in parallel, so they are independent of each other.
To be able to have a single function we need to pass some indicator from onChange event - which must be passed from the container as our CountrySelect component is generic. I think in the end we will have more not DRY code than we have now.
| import { CountryPerformanceOnRisk, CountrySelect } | ||
| from '../src/components/CountryPerformanceOnRisk.js'; | ||
| import React from 'react'; | ||
| import { Provider } from 'react-redux'; |
There was a problem hiding this comment.
We are not using Provider in this test. I forgot to remove it previously.
| } | ||
|
|
||
| it('Dropdown is there', () => { | ||
| const wrapper = shallow(< CountrySelect selectOptions={[]}/>) |
There was a problem hiding this comment.
Probably now, it would be better to use setup() function for wrapper:
const { enzymeWrapper } = setup()
expect(toJson(enzymeWrapper)).toMatchSnapshot();
| < CountrySelect | ||
| selectOptions={this.state.countries} | ||
| onChange={this.updateValue1} | ||
| selectedCountries={this.state.selected1} |
There was a problem hiding this comment.
Lets change the name to selectedCountry as it should be singular.
|
I've reviewed and this is excellent. Good things:
|
|
LGMT |
|
Merging. Closes #15 |
This PR makes it possible for graphs to select country from 3 different drop down menus and add or remove data for selected country.
Thought selecting/removing is done IMHO in a quite hacky way, this is the only way we found solution to handle changing countries from 3 different dropdowns and not mass with each other.
This PR is introducing the Redux store in application and using mocked up action and reducer to handle inserting and removing lines from graph. This is happening in
CountryPerformanceOnRiskcomponentAs
CountryPerformanceOnRiskhas become "smart" component and is handling graph modification, testing this becomes harder. This PR does include thorough (partially) tests checking weather onchange methods are called and state is updated. (meaning line is added to the graph)Testing smart component seems harder then expected. This may be helpful - reduxjs/redux#588
Besides this PR includes:
index.jsfile with mocked up reducer (responsible for adding/removing)