feat(environments): Add group events environment filtering #7366
Conversation
Generated by 🚫 danger |
lynnagara
force-pushed
the
group-events-env-filtering
branch
from
ac6c882 to
45194dc
Compare
| { | ||
| query: newQueryString, | ||
| }, | ||
| this.handleSearch(newQueryString) |
There was a problem hiding this comment.
Why setState here if handleSearch is going to navigate to a diff route. Also why call handleSearch in callback if it doesn't depend on state?
There was a problem hiding this comment.
Good catch, that's not necessary
| @@ -25,7 +25,7 @@ import StreamFilters from './stream/filters'; | |||
| import StreamSidebar from './stream/sidebar'; | |||
| import TimeSince from '../components/timeSince'; | |||
| import utils from '../utils'; | |||
| import streamUtils from './stream/utils'; | |||
| import qsUtils from '../utils/queryString'; | |||
There was a problem hiding this comment.
nit: let's try to keep import name the same as module name
| placeholder: PropTypes.string, | ||
| }; | ||
|
|
||
| static defaultProps = { | ||
| defaultQuery: '', | ||
| query: '', | ||
| onSearch: function() {}, | ||
| onQueryChange: function() {}, |
There was a problem hiding this comment.
What about the component passing onQueryChange to this? Was it not being passed?
There was a problem hiding this comment.
It wasn't passed or used anywhere
| nextProps.location.search !== this.props.location.search | ||
| ) { | ||
| let queryParams = nextProps.location.query; | ||
| // If query has changed, update the environment with the query environment |
There was a problem hiding this comment.
Is it intentional that this is no longer handling the case where groupId is changed?
There was a problem hiding this comment.
Yes, it's guarding against a situation that I can't see ever arising. We don't change group ID within the group details view. To change group ID you'd have to navigate to a different route and the component will be remounted.
There was a problem hiding this comment.
Seems I added it when upgrading to react-router 1.0 here. I don't really remember what I was doing; maybe I wasn't using routes correctly. I agree it should be re-mounted.
Add ability to filter group events by environment