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

Use prop instead of state for searchText #1160

Merged
merged 2 commits into from
Oct 21, 2016

Conversation

manisandro
Copy link
Contributor

Perhaps I missed the reason for storing searchText in the component state,
but it would be convenient for us to have the searchText field in the global
redux state reflect the actual search text to correctly restore the search
text from the url via a dispatch(searchTextChanged(text)) in loadMapConfig.

@simboss simboss added the ready label Oct 17, 2016
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.02%) to 80.554% when pulling ed061df on sourcepole:search_text_prop into ffbaac9 on geosolutions-it:master.

@mbarto
Copy link
Contributor

mbarto commented Oct 17, 2016

I agree. Please look at the failing tests

@manisandro
Copy link
Contributor Author

I suspect what's going on is that now that searchText is controlled by the global state and not by the component state, its value fails to update since action + reducer are not invoked in the test. Is this a possibility?

@mbarto
Copy link
Contributor

mbarto commented Oct 18, 2016

Yes, it's possible, we probably need to think of a different set of tests

@coveralls
Copy link

Coverage Status

Coverage increased (+0.2%) to 80.799% when pulling 971968a on sourcepole:search_text_prop into ffbaac9 on geosolutions-it:master.

Copy link
Contributor

@mbarto mbarto left a comment

Choose a reason for hiding this comment

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

I think we miss proper actions and reducers to handle text change, isn't it?

@manisandro
Copy link
Contributor Author

Well the existing actions TEXT_SEARCH_TEXT_CHANGE and TEXT_SEARCH_RESET handle the searchText state just fine. Do you mean these should be used in the test?

@mbarto
Copy link
Contributor

mbarto commented Oct 21, 2016

Oh, ok, so actions and reducers for text change already existed also if state was used for internal handling. Sorry about asking. I'll do a quick local test and merge.

@mbarto mbarto merged commit e2ef856 into geosolutions-it:master Oct 21, 2016
@manisandro manisandro deleted the search_text_prop branch October 21, 2016 10:09
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.

4 participants