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.

Sign up for GitHub

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

Reset all filters button on Events page #435

Merged
merged 16 commits into from
Jun 16, 2018
Merged

Reset all filters button on Events page #435

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
e5e0181
Initial commit with working reset button
jonyardley Jun 13, 2018
4ab0a28
Fix flow issue in tests rather than adding default prop for selectedC…
jonyardley Jun 14, 2018
162fbe8
Add some wee animation
jonyardley Jun 14, 2018
02c688f
Tidy up with constants and start test writing
jonyardley Jun 15, 2018
d8d6b99
Test to check onPress is clicked
jonyardley Jun 15, 2018
08999f9
Add test for not visible
jonyardley Jun 15, 2018
740f9d9
Add test to ensure that scroll to top method is called when reset all…
jonyardley Jun 15, 2018
e6a28c7
Added rest animation test
jonyardley Jun 15, 2018
ce17274
A bit more coveragé
jonyardley Jun 15, 2018
06d1e28
Merge branch 'master' into reset-all-filters
jonyardley Jun 15, 2018
4f79e3c
Addressed a few comments
jonyardley Jun 15, 2018
1a9bd19
Tweak tests
jonyardley Jun 15, 2018
f1176bc
Merge branch 'master' into reset-all-filters
declanslevin Jun 15, 2018
43da313
Move reset all filters inline function to instance function
Jun 16, 2018
11c94b6
Stop animation on unmount and fix unit tests
Jun 16, 2018
923aff7
Add tests for FilterHeader and EventsScreen component
Jun 16, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/actions/event-filters.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,4 +61,4 @@ export const clearStagedEventFilters = () => (

export const clearEventFilters = () => (
dispatch: Dispatch<EventFiltersAction>
) => dispatch({ type: "CLEAR__EVENT_FILTERS" });
Copy link
Contributor Author

Choose a reason for hiding this comment

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

🎺

) => dispatch({ type: "CLEAR_EVENT_FILTERS" });
4 changes: 4 additions & 0 deletions src/components/EventList.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -103,6 +103,7 @@ class EventList extends Component<Props, State> {
sectionSeparator = () => <View style={styles.sectionSeparator} />;

keyExtractor = getId;
sectionList = null;

renderItem = ({ item }: RenderItemInfo) => {
const {
Expand Down Expand Up @@ -168,6 +169,9 @@ class EventList extends Component<Props, State> {
refreshing={refreshing}
onRefresh={onRefresh}
windowSize={10}
ref={sectionList => {
this.sectionList = sectionList;
}}
/>
);
}
Expand Down
3 changes: 2 additions & 1 deletion src/constants/text.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -187,5 +187,6 @@ export default {
stageImage: "community"
}
]
}
},
resetAllFilters: "Reset all filters"
};
33 changes: 27 additions & 6 deletions src/screens/EventsScreen/FilterHeader.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,38 +13,59 @@ import {
} from "../../constants/colors";
import text from "../../constants/text";
import { formatDateRange } from "../../data/formatters";
import ResetAllFiltersButton from "./ResetAllFiltersButton";

export type Props = {
onFilterCategoriesPress: Function,
dateFilter: ?DateRange,
onFilterButtonPress: () => void,
onDateFilterButtonPress: () => void,
selectedCategories: Set<EventCategoryName>,
numTagFiltersSelected: number
numTagFiltersSelected: number,
resetAllFiltersPress: () => void,
scrollEventListToTop: () => void
};

class FilterHeader extends React.PureComponent<Props> {
static defaultProps = {
resetAllFiltersPress: () => {}
};

render() {
const {
dateFilter,
onFilterCategoriesPress,
selectedCategories,
onFilterButtonPress,
onDateFilterButtonPress,
numTagFiltersSelected
numTagFiltersSelected,
resetAllFiltersPress,
scrollEventListToTop
} = this.props;
const formattedDateFilter = dateFilter
? formatDateRange(dateFilter)
: text.selectDates;

const anyAppliedFilters: boolean =
!!dateFilter || numTagFiltersSelected > 0 || selectedCategories.size > 0;

return (
<View accessibilityTraits={["header"]} style={styles.container}>
<ContentPadding>
<View testID="event-filter-header" style={styles.content}>
<FilterHeaderCategories
onFilterPress={onFilterCategoriesPress}
selectedCategories={selectedCategories}
<View>
<ResetAllFiltersButton
visible={anyAppliedFilters}
onPress={() => {
Copy link
Contributor

Choose a reason for hiding this comment

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

If you make ResetAllFiltersButton a pure component, this shouldn't be an inline function.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Could you elaborate a bit? Not sure I follow this point.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

resetAllFiltersPress();
scrollEventListToTop();
}}
/>
<View testID="event-filter-header" style={styles.content}>
<FilterHeaderCategories
onFilterPress={onFilterCategoriesPress}
selectedCategories={selectedCategories}
/>
</View>
</View>
</ContentPadding>
<View style={styles.contentFilters}>
Expand Down
57 changes: 48 additions & 9 deletions src/screens/EventsScreen/FilterHeader.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import FilterHeader from "./FilterHeader";
import type { Props as ComponentProps } from "./FilterHeader";
import FilterHeaderButton from "./FilterHeaderButton";
import FilterHeaderCategories from "./FilterHeaderCategories";
import ResetAllFiltersButton from "./ResetAllFiltersButton";

const render = (
props: ComponentProps = {
Expand All @@ -13,7 +14,9 @@ const render = (
onFilterCategoriesPress: () => {},
onFilterButtonPress: () => {},
onDateFilterButtonPress: () => {},
numTagFiltersSelected: 0
resetAllFiltersPress: () => {},
numTagFiltersSelected: 0,
scrollEventListToTop: () => {}
}
) => shallow(<FilterHeader {...props} />);

Expand All @@ -25,7 +28,9 @@ describe("renders correctly", () => {
onFilterCategoriesPress: () => {},
onFilterButtonPress: () => {},
onDateFilterButtonPress: () => {},
numTagFiltersSelected: 0
resetAllFiltersPress: () => {},
numTagFiltersSelected: 0,
scrollEventListToTop: () => {}
});
expect(output).toMatchSnapshot();
});
Expand All @@ -37,7 +42,9 @@ describe("renders correctly", () => {
onFilterCategoriesPress: () => {},
onFilterButtonPress: () => {},
onDateFilterButtonPress: () => {},
numTagFiltersSelected: 0
resetAllFiltersPress: () => {},
numTagFiltersSelected: 0,
scrollEventListToTop: () => {}
});
expect(output).toMatchSnapshot();
});
Expand All @@ -49,7 +56,9 @@ describe("renders correctly", () => {
onFilterCategoriesPress: () => {},
onFilterButtonPress: () => {},
onDateFilterButtonPress: () => {},
numTagFiltersSelected: 0
resetAllFiltersPress: () => {},
numTagFiltersSelected: 0,
scrollEventListToTop: () => {}
});
expect(output).toMatchSnapshot();
});
Expand All @@ -64,7 +73,9 @@ describe("renders correctly", () => {
onFilterCategoriesPress: () => {},
onFilterButtonPress: () => {},
onDateFilterButtonPress: () => {},
numTagFiltersSelected: 0
resetAllFiltersPress: () => {},
numTagFiltersSelected: 0,
scrollEventListToTop: () => {}
});
expect(output).toMatchSnapshot();
});
Expand All @@ -76,7 +87,9 @@ describe("renders correctly", () => {
onFilterCategoriesPress: () => {},
onFilterButtonPress: () => {},
onDateFilterButtonPress: () => {},
numTagFiltersSelected: 2
resetAllFiltersPress: () => {},
numTagFiltersSelected: 2,
scrollEventListToTop: () => {}
});
expect(output).toMatchSnapshot();
});
Expand All @@ -91,7 +104,9 @@ describe("filter buttons", () => {
onFilterCategoriesPress: mock,
onFilterButtonPress: () => {},
onDateFilterButtonPress: () => {},
numTagFiltersSelected: 0
resetAllFiltersPress: () => {},
numTagFiltersSelected: 0,
scrollEventListToTop: () => {}
});
output.find(FilterHeaderCategories).prop("onFilterPress")();

Expand All @@ -106,7 +121,9 @@ describe("filter buttons", () => {
onFilterCategoriesPress: () => {},
onFilterButtonPress: () => {},
onDateFilterButtonPress: mock,
numTagFiltersSelected: 0
resetAllFiltersPress: () => {},
numTagFiltersSelected: 0,
scrollEventListToTop: () => {}
});
const button = output.find(FilterHeaderButton).at(0);
button.simulate("press");
Expand All @@ -122,11 +139,33 @@ describe("filter buttons", () => {
onFilterCategoriesPress: () => {},
onFilterButtonPress: mock,
onDateFilterButtonPress: () => {},
numTagFiltersSelected: 0
resetAllFiltersPress: () => {},
numTagFiltersSelected: 0,
scrollEventListToTop: () => {}
});
const button = output.find(FilterHeaderButton).at(1);
button.simulate("press");

expect(mock).toBeCalledWith();
});

it("calls scrollEventListToTop when users presses 'Reset all filters' button", () => {
const mock = jest.fn();
const output = render({
dateFilter: null,
selectedCategories: new Set(["Music"]),
onFilterCategoriesPress: () => {},
onFilterButtonPress: () => {},
onDateFilterButtonPress: () => {},
resetAllFiltersPress: () => {},
numTagFiltersSelected: 0,
scrollEventListToTop: mock
});
output
.find(ResetAllFiltersButton)
.props()
.onPress();

expect(mock).toBeCalledWith();
});
});
9 changes: 8 additions & 1 deletion src/screens/EventsScreen/FilterHeaderConnected.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import {
selectTagFilterSelectedCount
} from "../../selectors/event-filters";
import Component from "./FilterHeader";
import { clearEventFilters } from "../../actions/event-filters";

type OwnProps = {
onFilterCategoriesPress: Function,
Expand All @@ -32,6 +33,10 @@ const getNumTagFiltersSelected = createSelector(
selectTagFilterSelectedCount
);

type DispatchProps = {
resetAllFiltersPress: () => void
};

// Note we must add a return type here for react-redux connect to work
// with flow correctly. If not provided is silently fails if types do
// not line up. See https://github.com/facebook/flow/issues/5343
Expand All @@ -40,7 +45,9 @@ const mapStateToProps = (state: State): StateProps => ({
numTagFiltersSelected: getNumTagFiltersSelected(state)
});

const mapDispatchToProps = {};
const mapDispatchToProps = (dispatch): DispatchProps => ({
resetAllFiltersPress: () => dispatch(clearEventFilters())
});

const connector: Connector<OwnProps, Props> = connect(
mapStateToProps,
Expand Down
8 changes: 6 additions & 2 deletions src/screens/EventsScreen/FilterHeaderConnected.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,18 +35,22 @@ const initialState = {
}
};

const defaultProps = {
selectedCategories: new Set()
};

const mockStore = configureStore([thunk]);

describe("ConnectedFilterHeader", () => {
it("renders connector", () => {
const store = mockStore(initialState);
const output = shallow(<FilterHeader store={store} />);
const output = shallow(<FilterHeader store={store} {...defaultProps} />);
expect(output).toMatchSnapshot();
});

it("renders component", () => {
const store = mockStore(initialState);
const output = shallow(<FilterHeader store={store} />);
const output = shallow(<FilterHeader store={store} {...defaultProps} />);
expect(output.dive()).toMatchSnapshot();
});
});
113 changes: 113 additions & 0 deletions src/screens/EventsScreen/ResetAllFiltersButton.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,113 @@
// @flow
import React from "react";
import { Animated, StyleSheet, Easing } from "react-native";
import type { ViewLayoutEvent } from "react-native/Libraries/Components/View/ViewPropTypes";
import FilterHeaderButton from "./FilterHeaderButton";
import text from "../../constants/text";

type Props = {
visible: boolean,
onPress: () => void,
animationTime: number,
animationDelay: number
};

type State = {
isAnimating: boolean
};

const DEFAULT_HEIGHT = 120;
const DEFAULT_FADE_VALUE = 1;
const DEFAULT_TOP_OFFSET_VALUE = 0;

class ResetAllFiltersButton extends React.Component<Props, State> {
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you want to make this a PureComponent, so it doesn't rerender all the time?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Certainly!

static defaultProps = {
animationTime: 200,
animationDelay: 50
};

constructor(props: Props) {
super(props);
this.state = {
isAnimating: false
};
}

setButtonHeight = (e: ViewLayoutEvent): void => {
const { height } = e.nativeEvent.layout;
this.height = height;
};

fadeOut = (): void => {
const { animationTime, animationDelay } = this.props;

Animated.parallel([
Animated.timing(this.fadeValue, {
toValue: 0,
duration: animationTime / 2
}),
Animated.timing(this.topOffset, {
toValue: -this.height,
duration: animationTime,
easing: Easing.out(Easing.quad),
delay: animationDelay
})
]).start(this.animationFinished);
};

resetAllFilters = (): void => {
this.props.onPress();
this.setState({ isAnimating: true });
this.fadeOut();
};

resetAnimation = (): void => {
this.topOffset = new Animated.Value(DEFAULT_TOP_OFFSET_VALUE);
this.fadeValue = new Animated.Value(DEFAULT_FADE_VALUE);
};

animationFinished = (): void => {
this.setState({ isAnimating: false });
Copy link
Contributor

Choose a reason for hiding this comment

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

This might be called when the button has already been unmounted, in which case setState will throw an exception.

Copy link
Contributor Author

@jonyardley jonyardley Jun 15, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

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).

Copy link
Contributor Author

@jonyardley jonyardley Jun 15, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Looked into this a bit more and there is a post from Dan Abramov on this issue:

facebook/react#2787 (comment)

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

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

componentWillUnmount() {
this.completeAnimation();
}

Maybe our crash was unrelated though and we can just leave this at it is.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

this.resetAnimation();
};

topOffset: Animated.Value = new Animated.Value(DEFAULT_TOP_OFFSET_VALUE);
fadeValue: Animated.Value = new Animated.Value(DEFAULT_FADE_VALUE);
height: number = DEFAULT_HEIGHT;

render() {
const isVisible: boolean = this.state.isAnimating || this.props.visible;
return (
isVisible && (
<Animated.View
onLayout={this.setButtonHeight}
style={[
styles.clearAllWrapper,
{ opacity: this.fadeValue, marginTop: this.topOffset }
]}
>
<FilterHeaderButton
active={false}
text={text.resetAllFilters}
label={text.resetAllFilters}
style={styles.clearAll}
onPress={this.resetAllFilters}
/>
</Animated.View>
)
);
}
}

const styles = StyleSheet.create({
clearAll: {
minHeight: 0,
paddingTop: 16
},
clearAllWrapper: {
flexDirection: "row",
justifyContent: "flex-end"
}
});

export default ResetAllFiltersButton;
Loading