Reset all filters button on Events page #435
Conversation
| @@ -61,4 +61,4 @@ export const clearStagedEventFilters = () => ( | |||
|
|
|||
| export const clearEventFilters = () => ( | |||
| dispatch: Dispatch<EventFiltersAction> | |||
| ) => dispatch({ type: "CLEAR__EVENT_FILTERS" }); | |||
9bd835a to
162fbe8
Compare
12856db to
02c688f
Compare
| }; | ||
|
|
||
| animationFinished = (): void => { | ||
| this.setState({ isAnimating: false }); |
There was a problem hiding this comment.
This might be called when the button has already been unmounted, in which case setState will throw an exception.
There was a problem hiding this comment.
Ah yeah, good catch. I notice that isMounted() is now deprecated. Do you have a pattern in mind to catch this? Maybe a try catch (might be a bit crude).
There was a problem hiding this comment.
Looked into this a bit more and there is a post from Dan Abramov on this issue:
In any case, React doesn’t throw errors when this happens anymore. But we will intentionally keep the suboptimal case clunky so that the developers have incentives to implement the right solution (cancellation).
It is now only a warning and not an error so inclined to leave this as is due do it being a edgy edge case unless you think it will be a major issue?
There was a problem hiding this comment.
Hm, interesting. We tested this we @RGBboy for the heart animation. I'm pretty sure we could make this crash. 🤔
As far as I can see the recommendation is to set a field, which is basically what we ended up doing here:
pride-london-app/src/components/SaveEventButton.js
Lines 52 to 54 in 3a94242
Maybe our crash was unrelated though and we can just leave this at it is.
There was a problem hiding this comment.
I created a unit test for unmount -> setState and it threw an exception. So I just added a quick fix for this, just to be on the safe side.
| const DEFAULT_FADE_VALUE = 1; | ||
| const DEFAULT_TOP_OFFSET_VALUE = 0; | ||
|
|
||
| class ResetAllFiltersButton extends React.Component<Props, State> { |
There was a problem hiding this comment.
Do you want to make this a PureComponent, so it doesn't rerender all the time?
| <View> | ||
| <ResetAllFiltersButton | ||
| visible={anyAppliedFilters} | ||
| onPress={() => { |
There was a problem hiding this comment.
If you make ResetAllFiltersButton a pure component, this shouldn't be an inline function.
There was a problem hiding this comment.
Could you elaborate a bit? Not sure I follow this point.
There was a problem hiding this comment.
As fas as I know, this will generate a new function every time in render, which means that inside ResetAllFiltersButton this check will always be false: props.onPress === nextProps.onPress. So it will render every time.
We could solve this by adding the function to the FilterHeader class:
resetAllFilters = () => {
this.props.resetAllFiltersPress();
this.props.scrollEventListToTop();
};Then use onPress={this.resetAllFilters}. This will always pass in the same function.
|
The reset button has a weird touch area. It is mostly above the text. Do you think we are able to increase the touch area below the text? The highlighted part in the screenshot is the touchable area:
|
|
@frigus02 The padding this will be a bit tricky here. As we will need to animate the padding on the categories header too and would rather keep animation to a minimum. |
|
Ah, right. Makese sense with the animation 👍 |
This PR adds a
Reset all filtersbutton to theEventspage.The button should exhibit the following behaviours:
Test Plan
Following the steps outlined above should suffice.
Areas of risk
Further Notes
We have agreed that the header should probably be hidden when scrolling however due to a funky jitterbug with the scrolling and the refresh component we have decided to pull this feature out into a new issue/PR so that it can be tackled separately in order to get this out sooner.
Fixes: #340
Pre-flight check-list
Before raising a pull request
DocumentationPre-merge check-list
Tester check-list
Tested on multiple devices / OS versions (Test on the physical device(s) you have access to, otherwise use BrowserStack):
Optional:
Accessibility Testing (If applicable)
Weak connection testing (If applicable)