Skip to content

Use prop instead of state for searchText#1160

Merged
mbarto merged 2 commits into
geosolutions-it:masterfrom
sourcepole:search_text_prop
Oct 21, 2016
Merged

Use prop instead of state for searchText#1160
mbarto merged 2 commits into
geosolutions-it:masterfrom
sourcepole:search_text_prop

Conversation

@manisandro

Copy link
Copy Markdown
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
Copy Markdown

Coverage Status

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

@mbarto

mbarto commented Oct 17, 2016

Copy link
Copy Markdown
Contributor

I agree. Please look at the failing tests

@manisandro

Copy link
Copy Markdown
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

mbarto commented Oct 18, 2016

Copy link
Copy Markdown
Contributor

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

@coveralls

Copy link
Copy Markdown

Coverage Status

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

@mbarto mbarto left a comment

Copy link
Copy Markdown
Contributor

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
Copy Markdown
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

mbarto commented Oct 21, 2016

Copy link
Copy Markdown
Contributor

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