-
Notifications
You must be signed in to change notification settings - Fork 14
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
Agenda quick filters pills #609
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- fix merge conflicts
- specify the ticket number
- add a comment to the PR explaining why you didn't fulfill the checklist item regarding typescript types
I haven't checked the last checkbox because technically if I had to, I'd need to type the whole redux state for search & agenda, which I'm not going to right now. The other point is covered though, the component is typed properly with it's dispatch & store props. |
If it would involve lots of work here adding types to many places not related to this PR, I think we can conclude that it's not easy - thus checkpoint can be marked as completed anyway. |
import {connect} from 'react-redux'; | ||
import {agendaCoverageStatusFilter, getActiveFilterLabel} from 'agenda/components/AgendaCoverageExistsFilter'; | ||
|
||
const IS_AGENDA = location.pathname.includes('/agenda'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We've already talked that this is a low quality implementation that we'd normally not approve, but are going with it anyway because of effort that would be required to do a proper one. In such cases it'd be very useful to add a comment describing the situation and guidelines on how should we fix it when we have more time.
@@ -225,12 +231,7 @@ const mapStateToProps = (state: any) => ({ | |||
}); | |||
|
|||
const mapDispatchToProps = (dispatch: any) => ({ | |||
clearQuickFilter: (filter: string) => dispatch(clearQuickFilter(filter)), | |||
clearItemTypeFilter: () => dispatch(setItemTypeFilter(null)), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
move this one too
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fix ci
NHUB-414
Checklist