From 0b9f400092528239b00ab0f786272aadd8277c29 Mon Sep 17 00:00:00 2001 From: Kushan Joshi <0o3ko0@gmail.com> Date: Thu, 29 Jun 2017 15:16:39 +0530 Subject: [PATCH 1/4] use HOCs for keyboard shortcuts and data detching --- src/components/changeset/changeset_layout.js | 0 src/components/changeset/control_layout.js | 95 +++++++ src/components/changeset/index.js | 282 +++++-------------- src/components/changeset/user.js | 13 +- src/components/fetch_data_enhancer.js | 61 ++++ src/components/keyboard_enhancer.js | 67 +++++ src/config/bindings.js | 76 +++-- src/index.js | 1 - src/utils/component.js | 3 + src/utils/promise.js | 5 +- src/views/changeset.js | 8 +- src/views/changesets_list.js | 71 ++--- src/views/navbar_changeset.js | 88 +++--- 13 files changed, 460 insertions(+), 310 deletions(-) create mode 100644 src/components/changeset/changeset_layout.js create mode 100644 src/components/changeset/control_layout.js create mode 100644 src/components/fetch_data_enhancer.js create mode 100644 src/components/keyboard_enhancer.js create mode 100644 src/utils/component.js diff --git a/src/components/changeset/changeset_layout.js b/src/components/changeset/changeset_layout.js new file mode 100644 index 00000000..e69de29b diff --git a/src/components/changeset/control_layout.js b/src/components/changeset/control_layout.js new file mode 100644 index 00000000..c0d01b99 --- /dev/null +++ b/src/components/changeset/control_layout.js @@ -0,0 +1,95 @@ +import React from 'react'; +import { Map, List } from 'immutable'; + +import { Control } from './control'; +import { Floater } from './floater'; + +import { + CHANGESET_DETAILS_DETAILS, + CHANGESET_DETAILS_SUSPICIOUS, + CHANGESET_DETAILS_USER, + CHANGESET_DETAILS_DISCUSSIONS, + CHANGESET_DETAILS_MAP +} from '../../config/bindings'; + +export function ControlLayout({ + bindingsState, + features, + left, + discussions, + toggleDetails, + toggleFeatures, + toggleDiscussions, + toggleUser, + toggleMapOptions +}) { + return ( + + + + + + + + + + + + + + + + + + + + + + + + + + + + ); +} diff --git a/src/components/changeset/index.js b/src/components/changeset/index.js index e6f9b9e4..e849fba5 100644 --- a/src/components/changeset/index.js +++ b/src/components/changeset/index.js @@ -2,7 +2,6 @@ import React from 'react'; import { Map, List, fromJS } from 'immutable'; import CSSGroup from 'react-transition-group/CSSTransitionGroup'; -import Mousetrap from 'mousetrap'; import { getUserDetails } from '../../network/openstreetmap'; import { Floater } from './floater'; @@ -11,14 +10,13 @@ import { User } from './user'; import { Features } from './features'; import { Box } from './box'; import { Discussions } from './discussions'; -import { Control } from './control'; import { MapOptions } from './map_options'; - -import { cancelablePromise } from '../../utils/promise'; +import { ControlLayout } from './control_layout'; +import { keyboardToggleEnhancer } from '../keyboard_enhancer'; +import { withFetchDataSilent } from '../fetch_data_enhancer'; import { osmCommentsApi, whosThat } from '../../config/constants'; import { - CHANGESET_DETAILS_SHOW_ALL, CHANGESET_DETAILS_DETAILS, CHANGESET_DETAILS_SUSPICIOUS, CHANGESET_DETAILS_USER, @@ -27,113 +25,39 @@ import { } from '../../config/bindings'; // presentational component for view/changeset.js -export class Changeset extends React.PureComponent { +class Changeset extends React.PureComponent { state = { - width: 0, - left: 0, - discussions: false, - features: false, - user: false, - details: true, - mapOptions: false, - discussionsData: List(), - userDetails: new Map() + left: 0 + }; + static defaultProps = { + data: Map() }; props: { + data: Map, filterChangesetsByUser: () => any, changesetId: number, - currentChangeset: Map + currentChangeset: Map, + bindingsState: Map, + exclusiveKeyToggle: (label: string) => any }; ref = null; - getOsmCommentsPromise: Promise<*>; - getUserDetailsPromise: Promise<*>; - getWhosThatPromise: Promise<*>; - componentWillReceiveProps(nextProps: Object) { - if (this.props.changesetId !== nextProps.changesetId) - this.getData(nextProps.changesetId, nextProps.currentChangeset); - } + componentDidMount() { - Mousetrap.bind(CHANGESET_DETAILS_SUSPICIOUS, () => { - this.toggleFeatures(); - }); - Mousetrap.bind(CHANGESET_DETAILS_DISCUSSIONS, () => { - this.toggleDiscussions(); - }); - Mousetrap.bind(CHANGESET_DETAILS_DETAILS, () => { - this.toggleDetails(); - }); - Mousetrap.bind(CHANGESET_DETAILS_USER, () => { - this.toggleUser(); - }); - Mousetrap.bind(CHANGESET_DETAILS_MAP, () => { - this.toggleMapOptions(); - }); - this.getData(this.props.changesetId, this.props.currentChangeset); - } - componentWillUnmount() { - this.getOsmCommentsPromise && this.getOsmCommentsPromise.cancel(); - this.getUserDetailsPromise && this.getUserDetailsPromise.cancel(); + this.toggleDetails(); } + setRef = (r: any) => { if (!r) return; var rect = r.parentNode.parentNode.parentNode.getBoundingClientRect(); this.setState({ - width: parseInt(rect.width, 10), left: parseInt(rect.left, 10) }); }; - getData = (changesetId: number, currentChangeset: Map) => { - const uid: number = parseInt( - currentChangeset.getIn(['properties', 'uid']), - 10 - ); - const user: string = currentChangeset.getIn(['properties', 'user']); - const getUserDetailsPromise = cancelablePromise(getUserDetails(uid)); - const getOsmCommentsPromise = cancelablePromise( - fetch(`${osmCommentsApi}/${changesetId}`).then(r => r.json()) - ); - const getWhosThatPromise = cancelablePromise( - fetch(`${whosThat}${user}`).then(r => r.json()) - ); - - this.getUserDetailsPromise = getUserDetailsPromise; - this.getOsmCommentsPromise = getOsmCommentsPromise; - this.getWhosThatPromise = getWhosThatPromise; - - getUserDetailsPromise.promise - .then(userDetails => { - this.setState({ - userDetails: this.state.userDetails.merge(userDetails) - }); - }) - .catch(e => {}); - - getOsmCommentsPromise.promise - .then(x => { - if (x && x.properties && Array.isArray(x.properties.comments)) { - this.setState({ - discussionsData: fromJS(x.properties.comments) - }); - } - }) - .catch(e => {}); - - getWhosThatPromise.promise.then(d => { - console.log(d); - if (Array.isArray(d) && d[0] && d[0].names) { - let userDetails = new Map(); - userDetails = userDetails.set('otherNames', fromJS(d[0].names)); - this.setState({ - userDetails: this.state.userDetails.merge(userDetails) - }); - } - }); - }; showFloaters = () => { const { changesetId, currentChangeset } = this.props; + const bindingsState = this.props.bindingsState; const properties = currentChangeset.get('properties'); - return ( - {this.state.details && + {bindingsState.get(CHANGESET_DETAILS_DETAILS.label) &&
} - {this.state.features && + {bindingsState.get(CHANGESET_DETAILS_SUSPICIOUS.label) && } - {this.state.discussions && + {bindingsState.get(CHANGESET_DETAILS_DISCUSSIONS.label) && } - {this.state.user && + {bindingsState.get(CHANGESET_DETAILS_USER.label) && } - {this.state.mapOptions && + {bindingsState.get(CHANGESET_DETAILS_MAP.label) && } ); }; - toggleFeatures = () => { - this.setState({ - discussions: false, - details: false, - features: !this.state.features, - mapOptions: false, - user: false - }); + toggleFeatures = () => { + this.props.exclusiveKeyToggle(CHANGESET_DETAILS_SUSPICIOUS.label); }; toggleDiscussions = () => { - this.setState({ - discussions: !this.state.discussions, - details: false, - features: false, - mapOptions: false, - - user: false - }); + this.props.exclusiveKeyToggle(CHANGESET_DETAILS_DISCUSSIONS.label); }; toggleDetails = () => { - this.setState({ - discussions: false, - details: !this.state.details, - features: false, - mapOptions: false, - - user: false - }); + this.props.exclusiveKeyToggle(CHANGESET_DETAILS_DETAILS.label); }; toggleUser = () => { - this.setState({ - discussions: false, - details: false, - features: false, - mapOptions: false, - - user: !this.state.user - }); + this.props.exclusiveKeyToggle(CHANGESET_DETAILS_USER.label); }; toggleMapOptions = () => { - this.setState({ - discussions: false, - details: false, - features: false, - user: false, - mapOptions: !this.state.mapOptions - }); + this.props.exclusiveKeyToggle(CHANGESET_DETAILS_MAP.label); }; + render() { const features = this.props.currentChangeset.getIn([ 'properties', 'features' ]); + return (
- - - - - - - - - - - - - - - - - - - - - - - - - - - + getUserDetails(props.uid), + osmComments: props => + fetch(`${osmCommentsApi}/${props.changesetId}`).then(r => r.json()), + whosThat: props => fetch(`${whosThat}${props.user}`).then(r => r.json()) + }, + (nextProps, props) => props.changesetId !== nextProps.changesetId, + Changeset +); + +export { Changeset }; diff --git a/src/components/changeset/user.js b/src/components/changeset/user.js index 8b1ee4bd..cf1ab489 100644 --- a/src/components/changeset/user.js +++ b/src/components/changeset/user.js @@ -5,7 +5,12 @@ import AnchorifyText from 'react-anchorify-text'; import { Button } from '../button'; import AssemblyAnchor from '../assembly_anchor'; -export function User({ userDetails, filterChangesetsByUser }) { +export function User({ + userDetails, + osmComments, + whosThat, + filterChangesetsByUser +}) { return (

@@ -57,12 +62,10 @@ export function User({ userDetails, filterChangesetsByUser }) {

- {userDetails.has('otherNames') && - userDetails.get('otherNames').size > 1 && + {whosThat &&
Past usernames:   - {userDetails - .get('otherNames') + {whosThat .slice(0, -1) .map((e, k) => {e} )}
} diff --git a/src/components/fetch_data_enhancer.js b/src/components/fetch_data_enhancer.js new file mode 100644 index 00000000..8ede9dc5 --- /dev/null +++ b/src/components/fetch_data_enhancer.js @@ -0,0 +1,61 @@ +// @flow +import React from 'react'; +import { Map, fromJS } from 'immutable'; + +import { cancelablePromise } from '../utils/promise'; +import { getDisplayName } from '../utils/component'; + +// If any network request fails it silents it +// It also set states on first come first serve +// basis. +// @onUpdate a function which is equivalent to shouldComponentUpdate +// on which fetching should rework +export function withFetchDataSilent( + dataToFetch: Object, + onUpdate: (nextProps: Object, props: Object) => boolean, + WrappedComponent: Class> +) { + class FetchDataEnhancer extends React.PureComponent { + state = { + data: Map() + }; + static displayName = `HOCFetchData${getDisplayName(WrappedComponent)}`; + promises: Array<*>; + componentDidMount() { + this.initFetching(this.props); + } + + componentWillReceiveProps(nextProps) { + if (onUpdate(nextProps, this.props)) { + this.initFetching(nextProps); + } + } + initFetching(props) { + console.log('initialize fetching'); + const keys = Object.keys(dataToFetch); + // Collect array of promises, one for each api request + this.promises = keys.map(key => + cancelablePromise(dataToFetch[key](props)) + ); + this.promises.forEach((p, i) => { + p.promise + .then(x => { + console.log('zappening', keys[i]); + let data = this.state.data; + data = data.set(keys[i], fromJS(x)); + this.setState({ data }); + }) + .catch(e => console.error(e)); + }); + } + componentWillUnmount() { + console.log('unmounting'); + this.promises.forEach(p => p && p.cancel()); + } + render() { + return ; + } + } + + return FetchDataEnhancer; +} diff --git a/src/components/keyboard_enhancer.js b/src/components/keyboard_enhancer.js new file mode 100644 index 00000000..7609396d --- /dev/null +++ b/src/components/keyboard_enhancer.js @@ -0,0 +1,67 @@ +// @flow +import React from 'react'; +import { Map } from 'immutable'; + +import Mousetrap from 'mousetrap'; +import { getDisplayName } from '../utils/component'; + +export function keyboardToggleEnhancer( + exclusive: boolean, + bindings: Array<{ label: string, bindings: Array }>, + WrappedComponent: Class> +) { + return class wrapper extends React.PureComponent { + static displayName = `HOCKeyboard${getDisplayName(WrappedComponent)}`; + state = { bindings: Map() }; + componentDidMount() { + bindings.forEach(item => + Mousetrap.bind(item.bindings, () => { + if (exclusive) { + return this.exclusiveKeyToggle(item.label); + } + this.toggleKey(item.label); + }) + ); + } + + // allow toggling the state of a particular key + toggleKey = label => { + let prev = this.state.bindings; + prev = prev.set(label, !prev.get(label)); + this.setState({ + bindings: prev + }); + }; + + // exclusively toggle this label and switch off others + exclusiveKeyToggle = label => { + let newBindingState = Map(); + const prevBindingValue = this.state.bindings.get(label); + newBindingState = newBindingState.set(label, !prevBindingValue); + this.replaceKeysState(newBindingState); + }; + + // DANGEROUS! replaces the entire binding state with whatever is provided + replaceKeysState = (bindings: Map) => { + this.setState({ + bindings + }); + }; + + componentWillUnmount() { + // unbind all bindings + bindings.forEach(item => item.bindings.forEach(b => Mousetrap.unbind(b))); + } + render() { + return ( + + ); + } + }; +} diff --git a/src/config/bindings.js b/src/config/bindings.js index bc29eb8b..22876e79 100644 --- a/src/config/bindings.js +++ b/src/config/bindings.js @@ -1,18 +1,62 @@ // @flow -export const FILTER_BINDING = '\\'; -export const NEXT_CHANGESET = ['down', 'right', 'space']; -export const PREV_CHANGESET = ['up', 'left']; - -export const CHANGESET_DETAILS_SHOW_ALL = ['0']; -export const CHANGESET_DETAILS_DETAILS = ['1']; -export const CHANGESET_DETAILS_SUSPICIOUS = ['2']; -export const CHANGESET_DETAILS_DISCUSSIONS = ['3']; -export const CHANGESET_DETAILS_USER = ['4']; -export const CHANGESET_DETAILS_MAP = ['5']; -export const VERIFY_GOOD = ['G', 'g']; -export const VERIFY_BAD = ['B', 'b']; -export const VERIFY_CLEAR = ['C', 'c', 'u', 'U']; -export const OPEN_IN_JOSM = ['J', 'j']; -export const OPEN_IN_HDYC = ['H', 'h']; -export const FILTER_BY_USER = ['A', 'a']; +export const FILTER_BINDING = { + label: 'FILTER_BINDING', + bindings: ['\\'] +}; +export const NEXT_CHANGESET = { + label: 'NEXT_CHANGESET', + bindings: ['down', 'right', 'space'] +}; +export const PREV_CHANGESET = { + label: 'PREV_CHANGESET', + bindings: ['up', 'left'] +}; +export const CHANGESET_DETAILS_SHOW_ALL = { + label: 'CHANGESET_DETAILS_SHOW_ALL', + bindings: ['0'] +}; +export const CHANGESET_DETAILS_DETAILS = { + label: 'CHANGESET_DETAILS_DETAILS', + bindings: ['1'] +}; +export const CHANGESET_DETAILS_SUSPICIOUS = { + label: 'CHANGESET_DETAILS_SUSPICIOUS', + bindings: ['2'] +}; +export const CHANGESET_DETAILS_DISCUSSIONS = { + label: 'CHANGESET_DETAILS_DISCUSSIONS', + bindings: ['3'] +}; +export const CHANGESET_DETAILS_USER = { + label: 'CHANGESET_DETAILS_USER', + bindings: ['4'] +}; +export const CHANGESET_DETAILS_MAP = { + label: 'CHANGESET_DETAILS_MAP', + bindings: ['5'] +}; +export const VERIFY_GOOD = { + label: 'VERIFY_GOOD', + bindings: ['G', 'g'] +}; +export const VERIFY_BAD = { + label: 'VERIFY_BAD', + bindings: ['B', 'b'] +}; +export const VERIFY_CLEAR = { + label: 'VERIFY_CLEAR', + bindings: ['C', 'c', 'u', 'U'] +}; +export const OPEN_IN_JOSM = { + label: 'OPEN_IN_JOSM', + bindings: ['J', 'j'] +}; +export const OPEN_IN_HDYC = { + label: 'OPEN_IN_HDYC', + bindings: ['H', 'h'] +}; +export const FILTER_BY_USER = { + label: 'FILTER_BY_USER', + bindings: ['A', 'a'] +}; diff --git a/src/index.js b/src/index.js index 63e6ad4b..91743530 100644 --- a/src/index.js +++ b/src/index.js @@ -9,7 +9,6 @@ import Raven from 'raven-js'; import { history } from './store/history'; import { store } from './store'; import { isDev, stack, appVersion } from './config'; - import { registerServiceWorker } from './serviceworker'; import './assets/index.css'; diff --git a/src/utils/component.js b/src/utils/component.js new file mode 100644 index 00000000..bb034b17 --- /dev/null +++ b/src/utils/component.js @@ -0,0 +1,3 @@ +export function getDisplayName(WrappedComponent) { + return WrappedComponent.displayName || WrappedComponent.name || 'Component'; +} diff --git a/src/utils/promise.js b/src/utils/promise.js index fbdd63b8..5be14328 100644 --- a/src/utils/promise.js +++ b/src/utils/promise.js @@ -1,7 +1,6 @@ // @flow -export function cancelablePromise( - promise: Promise<*> -): { promise: Promise<*>, cancel: () => any } { +export type cancelablePromiseType = { promise: Promise<*>, cancel: () => any }; +export function cancelablePromise(promise: Promise<*>): cancelablePromiseType { let hasCanceled_ = false; const wrappedPromise = new Promise((resolve, reject) => { diff --git a/src/views/changeset.js b/src/views/changeset.js index 0df8884c..067eee1b 100644 --- a/src/views/changeset.js +++ b/src/views/changeset.js @@ -1,7 +1,7 @@ // @flow import React from 'react'; import { connect } from 'react-redux'; -import { List as ImmutableList, Map, fromJS } from 'immutable'; +import { Map, fromJS } from 'immutable'; import Mousetrap from 'mousetrap'; import { Changeset as ChangesetDumb } from '../components/changeset'; @@ -22,10 +22,10 @@ class Changeset extends React.PureComponent { applyFilters: (Map) => mixed // base 0 }; componentDidMount() { - Mousetrap.bind(FILTER_BY_USER, this.filterChangesetsByUser); + Mousetrap.bind(FILTER_BY_USER.bindings, this.filterChangesetsByUser); } componentWillUnmount() { - FILTER_BY_USER.forEach(k => Mousetrap.unbind(k)); + FILTER_BY_USER.bindings.forEach(k => Mousetrap.unbind(k)); } filterChangesetsByUser = () => { if (this.props.currentChangeset) { @@ -69,6 +69,8 @@ class Changeset extends React.PureComponent { } return ( , - userDetails: Map, diff: number, diffLoading: boolean, pageIndex: number, activeChangesetId: ?number, - oAuthToken: ?string, - token: ?string, filters: Map>, + bindingsState: Map, getChangesetsPage: (number, ?boolean) => mixed, // base 0 - getOAuthToken: () => mixed, - getFinalToken: string => mixed, - logUserOut: () => mixed, push: Object => mixed, applyFilters: (Map>) => mixed // base 0 }; @@ -65,6 +55,7 @@ class ChangesetsList extends React.PureComponent { super(props); this.props.getChangesetsPage(props.pageIndex); } + goUpDownToChangeset = (direction: number) => { if (!this.props.currentPage) return; let features = this.props.currentPage.get('features'); @@ -83,8 +74,14 @@ class ChangesetsList extends React.PureComponent { } } }; - componentDidMount() { - Mousetrap.bind(FILTER_BINDING, () => { + + componentWillReceiveProps(nextProps) { + const bindingsState = this.props.bindingsState; + const nextBindingsState = nextProps.bindingsState; + if ( + bindingsState.get(FILTER_BINDING.label) !== + nextBindingsState.get(FILTER_BINDING.label) + ) { if (this.props.location && this.props.location.pathname === '/filters') { const location = { ...this.props.location, // clone it @@ -98,16 +95,21 @@ class ChangesetsList extends React.PureComponent { }; this.props.push(location); } - }); - Mousetrap.bind(NEXT_CHANGESET, e => { - e.preventDefault(); + } + if ( + bindingsState.get(NEXT_CHANGESET.label) !== + nextBindingsState.get(NEXT_CHANGESET.label) + ) { this.goUpDownToChangeset(1); - }); - Mousetrap.bind(PREV_CHANGESET, e => { - e.preventDefault(); + } + if ( + bindingsState.get(PREV_CHANGESET.label) !== + nextBindingsState.get(PREV_CHANGESET.label) + ) { this.goUpDownToChangeset(-1); - }); + } } + handleFilterOrderBy = (selected: Array<*>) => { let mergedFilters; mergedFilters = this.props.filters.set('order_by', fromJS(selected)); @@ -232,29 +234,28 @@ class ChangesetsList extends React.PureComponent { } } +ChangesetsList = keyboardToggleEnhancer( + false, + [NEXT_CHANGESET, PREV_CHANGESET, FILTER_BINDING], + ChangesetsList +); + ChangesetsList = connect( (state: RootStateType, props) => ({ - routing: state.routing, location: state.routing.location, - currentPage: state.changesetsPage.get('currentPage'), - pageIndex: state.changesetsPage.get('pageIndex') || 0, - diffLoading: state.changesetsPage.get('diffLoading'), - filters: state.changesetsPage.get('filters') || new Map(), - diff: state.changesetsPage.get('diff'), loading: state.changesetsPage.get('loading'), error: state.changesetsPage.get('error'), - oAuthToken: state.auth.get('oAuthToken'), - userDetails: state.auth.get('userDetails'), - token: state.auth.get('token'), - activeChangesetId: state.changeset.get('changesetId') + currentPage: state.changesetsPage.get('currentPage'), + diff: state.changesetsPage.get('diff'), + diffLoading: state.changesetsPage.get('diffLoading'), + pageIndex: state.changesetsPage.get('pageIndex') || 0, + activeChangesetId: state.changeset.get('changesetId'), + filters: state.changesetsPage.get('filters') || Map() }), { // actions getChangesetsPage, - getOAuthToken, - getFinalToken, applyFilters, - logUserOut, push } )(ChangesetsList); diff --git a/src/views/navbar_changeset.js b/src/views/navbar_changeset.js index 4ca517dc..decc3203 100644 --- a/src/views/navbar_changeset.js +++ b/src/views/navbar_changeset.js @@ -2,8 +2,8 @@ import React from 'react'; import { connect } from 'react-redux'; import { Map } from 'immutable'; -import Mousetrap from 'mousetrap'; +import { keyboardToggleEnhancer } from '../components/keyboard_enhancer'; import { Tags } from '../components/changeset/tags'; import { Link } from 'react-router-dom'; import { Navbar } from '../components/navbar'; @@ -42,55 +42,57 @@ class NavbarChangeset extends React.PureComponent { boolean | -1 ) => mixed }; - componentDidMount() { - Mousetrap.bind(VERIFY_BAD, () => { - this.props.currentChangeset && - this.props.handleChangesetModifyHarmful( - this.props.changesetId, - this.props.currentChangeset, - true - ); - }); - Mousetrap.bind(VERIFY_CLEAR, () => { - this.props.currentChangeset && - this.props.handleChangesetModifyHarmful( - this.props.changesetId, - this.props.currentChangeset, - -1 - ); - }); - Mousetrap.bind(VERIFY_GOOD, () => { - this.props.currentChangeset && - this.props.handleChangesetModifyHarmful( - this.props.changesetId, - this.props.currentChangeset, - false - ); - }); - Mousetrap.bind(OPEN_IN_JOSM, () => { + componentWillReceiveProps(nextProps) { + const bindingsState = this.props.bindingsState; + const nextBindingsState = nextProps.bindingsState; + + if (!this.props.currentChangeset) return; + if ( + bindingsState.get(VERIFY_BAD.label) !== + nextBindingsState.get(VERIFY_BAD.label) + ) { + this.props.handleChangesetModifyHarmful( + this.props.changesetId, + this.props.currentChangeset, + true + ); + } else if ( + bindingsState.get(VERIFY_CLEAR.label) !== + nextBindingsState.get(VERIFY_CLEAR.label) + ) { + this.props.handleChangesetModifyHarmful( + this.props.changesetId, + this.props.currentChangeset, + -1 + ); + } else if ( + bindingsState.get(VERIFY_GOOD.label) !== + nextBindingsState.get(VERIFY_GOOD.label) + ) { + this.props.handleChangesetModifyHarmful( + this.props.changesetId, + this.props.currentChangeset, + false + ); + } else if ( + bindingsState.get(OPEN_IN_JOSM.label) !== + nextBindingsState.get(OPEN_IN_JOSM.label) + ) { if (!this.props.changesetId) return; const url = `https://127.0.0.1:8112/import?url=http://www.openstreetmap.org/api/0.6/changeset/${this .props.changesetId}/download`; window.open(url, '_blank'); - }); - Mousetrap.bind(OPEN_IN_HDYC, () => { - if (!this.props.currentChangeset) return; + } else if ( + bindingsState.get(OPEN_IN_HDYC.label) !== + nextBindingsState.get(OPEN_IN_HDYC.label) + ) { const user: string = this.props.currentChangeset.getIn( ['properties', 'user'], '' ); const url = `http://hdyc.neis-one.org/?${user}`; window.open(url, '_blank'); - }); - } - componentWillUnmount() { - [ - ...VERIFY_BAD, - ...VERIFY_GOOD, - ...VERIFY_GOOD, - ...OPEN_IN_JOSM, - ...OPEN_IN_HDYC - ].forEach(k => Mousetrap.unbind(k)); + } } handleVerify = (arr: Array) => { if (arr.length === 1) { @@ -200,6 +202,12 @@ class NavbarChangeset extends React.PureComponent { } } +NavbarChangeset = keyboardToggleEnhancer( + false, + [VERIFY_BAD, VERIFY_GOOD, VERIFY_CLEAR, OPEN_IN_JOSM, OPEN_IN_HDYC], + NavbarChangeset +); + NavbarChangeset = connect( (state: RootStateType, props) => ({ location: props.location, From fe3fecfd5059ef43a2907e46d8c96efdf688cd5a Mon Sep 17 00:00:00 2001 From: Kushan Joshi <0o3ko0@gmail.com> Date: Thu, 29 Jun 2017 17:40:55 +0530 Subject: [PATCH 2/4] add lastkeystroke --- src/components/keyboard_enhancer.js | 27 ++++---- src/views/changesets_list.js | 68 ++++++++++---------- src/views/navbar_changeset.js | 97 ++++++++++++++--------------- 3 files changed, 97 insertions(+), 95 deletions(-) diff --git a/src/components/keyboard_enhancer.js b/src/components/keyboard_enhancer.js index 7609396d..bd70df57 100644 --- a/src/components/keyboard_enhancer.js +++ b/src/components/keyboard_enhancer.js @@ -1,10 +1,10 @@ // @flow import React from 'react'; -import { Map } from 'immutable'; +import { Map, OrderedMap } from 'immutable'; import Mousetrap from 'mousetrap'; import { getDisplayName } from '../utils/component'; - +window.O = OrderedMap; export function keyboardToggleEnhancer( exclusive: boolean, bindings: Array<{ label: string, bindings: Array }>, @@ -12,10 +12,12 @@ export function keyboardToggleEnhancer( ) { return class wrapper extends React.PureComponent { static displayName = `HOCKeyboard${getDisplayName(WrappedComponent)}`; - state = { bindings: Map() }; + state = { bindings: Map(), lastKeyStroke: Map() }; + componentDidMount() { bindings.forEach(item => - Mousetrap.bind(item.bindings, () => { + Mousetrap.bind(item.bindings, (e: Event) => { + e.preventDefault(); if (exclusive) { return this.exclusiveKeyToggle(item.label); } @@ -27,24 +29,23 @@ export function keyboardToggleEnhancer( // allow toggling the state of a particular key toggleKey = label => { let prev = this.state.bindings; + let lastKeyStroke = Map().set(label, !prev.get(label)); prev = prev.set(label, !prev.get(label)); this.setState({ - bindings: prev + bindings: prev, + lastKeyStroke }); }; // exclusively toggle this label and switch off others - exclusiveKeyToggle = label => { + exclusiveKeyToggle = (label: string) => { let newBindingState = Map(); const prevBindingValue = this.state.bindings.get(label); newBindingState = newBindingState.set(label, !prevBindingValue); - this.replaceKeysState(newBindingState); - }; - - // DANGEROUS! replaces the entire binding state with whatever is provided - replaceKeysState = (bindings: Map) => { + // !replaces the entire binding state this.setState({ - bindings + bindings: newBindingState, + lastKeyStroke: newBindingState // since it will be a Map of 1 recent key stroke }); }; @@ -57,7 +58,7 @@ export function keyboardToggleEnhancer( diff --git a/src/views/changesets_list.js b/src/views/changesets_list.js index dfb2047b..5ed8c2a1 100644 --- a/src/views/changesets_list.js +++ b/src/views/changesets_list.js @@ -1,7 +1,7 @@ // @flow import React from 'react'; import { connect } from 'react-redux'; -import { List as ImmutableList, Map, fromJS } from 'immutable'; +import { is, List as ImmutableList, Map, fromJS } from 'immutable'; import { NavLink } from 'react-router-dom'; import { push } from 'react-router-redux'; import numberWithCommas from '../utils/number_with_commas.js'; @@ -45,7 +45,7 @@ class ChangesetsList extends React.PureComponent { pageIndex: number, activeChangesetId: ?number, filters: Map>, - bindingsState: Map, + lastKeyStroke: Map, getChangesetsPage: (number, ?boolean) => mixed, // base 0 push: Object => mixed, applyFilters: (Map>) => mixed // base 0 @@ -74,39 +74,41 @@ class ChangesetsList extends React.PureComponent { } } }; - + toggleFilters() { + if (this.props.location && this.props.location.pathname === '/filters') { + const location = { + ...this.props.location, // clone it + pathname: '/' + }; + this.props.push(location); + } else { + const location = { + ...this.props.location, // clone it + pathname: '/filters' + }; + this.props.push(location); + } + } componentWillReceiveProps(nextProps) { - const bindingsState = this.props.bindingsState; - const nextBindingsState = nextProps.bindingsState; - if ( - bindingsState.get(FILTER_BINDING.label) !== - nextBindingsState.get(FILTER_BINDING.label) - ) { - if (this.props.location && this.props.location.pathname === '/filters') { - const location = { - ...this.props.location, // clone it - pathname: '/' - }; - this.props.push(location); - } else { - const location = { - ...this.props.location, // clone it - pathname: '/filters' - }; - this.props.push(location); + const lastKeyStroke: Map = nextProps.lastKeyStroke; + if (is(this.props.lastKeyStroke, lastKeyStroke)) return; + var string = lastKeyStroke.keySeq().first(); + switch (string) { + case FILTER_BINDING.label: { + this.toggleFilters(); + break; + } + case NEXT_CHANGESET.label: { + this.goUpDownToChangeset(1); + break; + } + case PREV_CHANGESET.label: { + this.goUpDownToChangeset(-1); + break; + } + default: { + return; } - } - if ( - bindingsState.get(NEXT_CHANGESET.label) !== - nextBindingsState.get(NEXT_CHANGESET.label) - ) { - this.goUpDownToChangeset(1); - } - if ( - bindingsState.get(PREV_CHANGESET.label) !== - nextBindingsState.get(PREV_CHANGESET.label) - ) { - this.goUpDownToChangeset(-1); } } diff --git a/src/views/navbar_changeset.js b/src/views/navbar_changeset.js index decc3203..70104dce 100644 --- a/src/views/navbar_changeset.js +++ b/src/views/navbar_changeset.js @@ -1,7 +1,7 @@ // @flow import React from 'react'; import { connect } from 'react-redux'; -import { Map } from 'immutable'; +import { is, Map } from 'immutable'; import { keyboardToggleEnhancer } from '../components/keyboard_enhancer'; import { Tags } from '../components/changeset/tags'; @@ -30,6 +30,7 @@ class NavbarChangeset extends React.PureComponent { location: Object, currentChangeset: Map, username: ?string, + lastKeyStroke: Map, handleChangesetModifyTag: ( number, Map, @@ -43,55 +44,53 @@ class NavbarChangeset extends React.PureComponent { ) => mixed }; componentWillReceiveProps(nextProps) { - const bindingsState = this.props.bindingsState; - const nextBindingsState = nextProps.bindingsState; - if (!this.props.currentChangeset) return; - if ( - bindingsState.get(VERIFY_BAD.label) !== - nextBindingsState.get(VERIFY_BAD.label) - ) { - this.props.handleChangesetModifyHarmful( - this.props.changesetId, - this.props.currentChangeset, - true - ); - } else if ( - bindingsState.get(VERIFY_CLEAR.label) !== - nextBindingsState.get(VERIFY_CLEAR.label) - ) { - this.props.handleChangesetModifyHarmful( - this.props.changesetId, - this.props.currentChangeset, - -1 - ); - } else if ( - bindingsState.get(VERIFY_GOOD.label) !== - nextBindingsState.get(VERIFY_GOOD.label) - ) { - this.props.handleChangesetModifyHarmful( - this.props.changesetId, - this.props.currentChangeset, - false - ); - } else if ( - bindingsState.get(OPEN_IN_JOSM.label) !== - nextBindingsState.get(OPEN_IN_JOSM.label) - ) { - if (!this.props.changesetId) return; - const url = `https://127.0.0.1:8112/import?url=http://www.openstreetmap.org/api/0.6/changeset/${this - .props.changesetId}/download`; - window.open(url, '_blank'); - } else if ( - bindingsState.get(OPEN_IN_HDYC.label) !== - nextBindingsState.get(OPEN_IN_HDYC.label) - ) { - const user: string = this.props.currentChangeset.getIn( - ['properties', 'user'], - '' - ); - const url = `http://hdyc.neis-one.org/?${user}`; - window.open(url, '_blank'); + const lastKeyStroke: Map = nextProps.lastKeyStroke; + if (is(this.props.lastKeyStroke, lastKeyStroke)) return; + switch (lastKeyStroke.keySeq().first()) { + case VERIFY_BAD.label: { + this.props.handleChangesetModifyHarmful( + this.props.changesetId, + this.props.currentChangeset, + true + ); + break; + } + case VERIFY_CLEAR.label: { + this.props.handleChangesetModifyHarmful( + this.props.changesetId, + this.props.currentChangeset, + -1 + ); + break; + } + case VERIFY_GOOD.label: { + this.props.handleChangesetModifyHarmful( + this.props.changesetId, + this.props.currentChangeset, + false + ); + break; + } + case OPEN_IN_JOSM.label: { + if (!this.props.changesetId) return; + const url = `https://127.0.0.1:8112/import?url=http://www.openstreetmap.org/api/0.6/changeset/${this + .props.changesetId}/download`; + window.open(url, '_blank'); + break; + } + case OPEN_IN_HDYC.label: { + const user: string = this.props.currentChangeset.getIn( + ['properties', 'user'], + '' + ); + const url = `http://hdyc.neis-one.org/?${user}`; + window.open(url, '_blank'); + break; + } + default: { + return; + } } } handleVerify = (arr: Array) => { From 22ec34f524a6528befdcf14c25f0a9fdc9328bf7 Mon Sep 17 00:00:00 2001 From: Kushan Joshi <0o3ko0@gmail.com> Date: Thu, 29 Jun 2017 18:18:57 +0530 Subject: [PATCH 3/4] minor flow fix --- package.json | 1 + src/components/changeset/changeset_layout.js | 0 src/components/fetch_data_enhancer.js | 4 ++-- src/components/keyboard_enhancer.js | 2 +- yarn.lock | 4 ++++ 5 files changed, 8 insertions(+), 3 deletions(-) delete mode 100644 src/components/changeset/changeset_layout.js diff --git a/package.json b/package.json index 6aa43213..ee5e7782 100644 --- a/package.json +++ b/package.json @@ -36,6 +36,7 @@ "redux-devtools-extension": "^2.13.2", "redux-logger": "^3.0.6", "redux-saga": "^0.15.3", + "reselect": "^3.0.1", "showdown": "^1.7.1", "superagent": "^3.5.2" }, diff --git a/src/components/changeset/changeset_layout.js b/src/components/changeset/changeset_layout.js deleted file mode 100644 index e69de29b..00000000 diff --git a/src/components/fetch_data_enhancer.js b/src/components/fetch_data_enhancer.js index 8ede9dc5..20d310d8 100644 --- a/src/components/fetch_data_enhancer.js +++ b/src/components/fetch_data_enhancer.js @@ -25,12 +25,12 @@ export function withFetchDataSilent( this.initFetching(this.props); } - componentWillReceiveProps(nextProps) { + componentWillReceiveProps(nextProps: Object) { if (onUpdate(nextProps, this.props)) { this.initFetching(nextProps); } } - initFetching(props) { + initFetching(props: Object) { console.log('initialize fetching'); const keys = Object.keys(dataToFetch); // Collect array of promises, one for each api request diff --git a/src/components/keyboard_enhancer.js b/src/components/keyboard_enhancer.js index bd70df57..f4ed26ca 100644 --- a/src/components/keyboard_enhancer.js +++ b/src/components/keyboard_enhancer.js @@ -27,7 +27,7 @@ export function keyboardToggleEnhancer( } // allow toggling the state of a particular key - toggleKey = label => { + toggleKey = (label: string) => { let prev = this.state.bindings; let lastKeyStroke = Map().set(label, !prev.get(label)); prev = prev.set(label, !prev.get(label)); diff --git a/yarn.lock b/yarn.lock index 71bdef2e..46f81db3 100644 --- a/yarn.lock +++ b/yarn.lock @@ -6340,6 +6340,10 @@ requires-port@1.0.x, requires-port@1.x.x: version "1.0.0" resolved "https://registry.yarnpkg.com/requires-port/-/requires-port-1.0.0.tgz#925d2601d39ac485e091cf0da5c6e694dc3dcaff" +reselect@^3.0.1: + version "3.0.1" + resolved "https://registry.yarnpkg.com/reselect/-/reselect-3.0.1.tgz#efdaa98ea7451324d092b2b2163a6a1d7a9a2147" + reserved-words@^0.1.1: version "0.1.1" resolved "https://registry.yarnpkg.com/reserved-words/-/reserved-words-0.1.1.tgz#6f7c15e5e5614c50da961630da46addc87c0cef2" From 08969ca1cd0ac7181213f24d8a7275ef1505212e Mon Sep 17 00:00:00 2001 From: Kushan Joshi <0o3ko0@gmail.com> Date: Mon, 3 Jul 2017 14:38:13 +0530 Subject: [PATCH 4/4] refactor changeset list --- src/components/fetch_data_enhancer.js | 23 ++++------ src/components/keyboard_enhancer.js | 24 +++++++---- src/components/list/footer.js | 56 ++++++++++++++++++++++++ src/components/list/index.js | 44 +++++++++---------- src/components/loading_enhancer.js | 8 ++++ src/utils/promise.js | 4 +- src/views/changesets_list.js | 61 +++++++-------------------- 7 files changed, 127 insertions(+), 93 deletions(-) create mode 100644 src/components/list/footer.js create mode 100644 src/components/loading_enhancer.js diff --git a/src/components/fetch_data_enhancer.js b/src/components/fetch_data_enhancer.js index 20d310d8..4ed98f7d 100644 --- a/src/components/fetch_data_enhancer.js +++ b/src/components/fetch_data_enhancer.js @@ -6,25 +6,24 @@ import { cancelablePromise } from '../utils/promise'; import { getDisplayName } from '../utils/component'; // If any network request fails it silents it -// It also set states on first come first serve -// basis. -// @onUpdate a function which is equivalent to shouldComponentUpdate -// on which fetching should rework +// It also sets state on first come first serve basis. +// @onUpdate a function to be passed by the invoker +// which decides whether to reload the network +// requests. export function withFetchDataSilent( dataToFetch: Object, - onUpdate: (nextProps: Object, props: Object) => boolean, - WrappedComponent: Class> + onUpdate: (Object, Object) => boolean, + WrappedComponent: any ) { - class FetchDataEnhancer extends React.PureComponent { + return class FetchDataEnhancer extends React.PureComponent { + static displayName = `HOCFetchData${getDisplayName(WrappedComponent)}`; state = { data: Map() }; - static displayName = `HOCFetchData${getDisplayName(WrappedComponent)}`; promises: Array<*>; componentDidMount() { this.initFetching(this.props); } - componentWillReceiveProps(nextProps: Object) { if (onUpdate(nextProps, this.props)) { this.initFetching(nextProps); @@ -40,7 +39,6 @@ export function withFetchDataSilent( this.promises.forEach((p, i) => { p.promise .then(x => { - console.log('zappening', keys[i]); let data = this.state.data; data = data.set(keys[i], fromJS(x)); this.setState({ data }); @@ -49,13 +47,10 @@ export function withFetchDataSilent( }); } componentWillUnmount() { - console.log('unmounting'); this.promises.forEach(p => p && p.cancel()); } render() { return ; } - } - - return FetchDataEnhancer; + }; } diff --git a/src/components/keyboard_enhancer.js b/src/components/keyboard_enhancer.js index f4ed26ca..ae4d0af7 100644 --- a/src/components/keyboard_enhancer.js +++ b/src/components/keyboard_enhancer.js @@ -1,10 +1,15 @@ // @flow import React from 'react'; -import { Map, OrderedMap } from 'immutable'; - import Mousetrap from 'mousetrap'; +import { Map } from 'immutable'; + import { getDisplayName } from '../utils/component'; -window.O = OrderedMap; + +/** + * @param exclusive - flag to toggle one key and switch off all other keys + * @prop `bindingsState` - immutable Map containing toggleState(true/false) for each of the bindings. + * @prop `lastKeyStroke` - immutable Map containg the state of last key pressed only. + */ export function keyboardToggleEnhancer( exclusive: boolean, bindings: Array<{ label: string, bindings: Array }>, @@ -12,6 +17,7 @@ export function keyboardToggleEnhancer( ) { return class wrapper extends React.PureComponent { static displayName = `HOCKeyboard${getDisplayName(WrappedComponent)}`; + state = { bindings: Map(), lastKeyStroke: Map() }; componentDidMount() { @@ -26,6 +32,11 @@ export function keyboardToggleEnhancer( ); } + componentWillUnmount() { + // unbind all bindings + bindings.forEach(item => item.bindings.forEach(b => Mousetrap.unbind(b))); + } + // allow toggling the state of a particular key toggleKey = (label: string) => { let prev = this.state.bindings; @@ -42,17 +53,12 @@ export function keyboardToggleEnhancer( let newBindingState = Map(); const prevBindingValue = this.state.bindings.get(label); newBindingState = newBindingState.set(label, !prevBindingValue); - // !replaces the entire binding state this.setState({ bindings: newBindingState, - lastKeyStroke: newBindingState // since it will be a Map of 1 recent key stroke + lastKeyStroke: newBindingState // will be same as state.bindings as size=1 }); }; - componentWillUnmount() { - // unbind all bindings - bindings.forEach(item => item.bindings.forEach(b => Mousetrap.unbind(b))); - } render() { return ( i + start); +} + +export function Footer({ + pageIndex, + getChangesetsPage, + count +}: { + pageIndex: number, + getChangesetsPage: (number, ?boolean) => mixed, + count: ?number +}) { + const base = parseInt(pageIndex / RANGE, 10) * RANGE; + let maxPageCount = 0; + if (count && !Number.isNaN(count)) { + maxPageCount = Math.ceil(count / PAGE_SIZE); + } + console.log(pageIndex, count, maxPageCount, base); + return ( +
+ + {range(base, Math.min(base + RANGE, maxPageCount)).map(n => + + )} + = maxPageCount} + pageIndex={pageIndex + 1} + active={false} + getChangesetsPage={getChangesetsPage} + /> +
+ ); +} diff --git a/src/components/list/index.js b/src/components/list/index.js index 0f2caeb3..072d6d8a 100644 --- a/src/components/list/index.js +++ b/src/components/list/index.js @@ -4,17 +4,16 @@ import { List as ImmutableList, Map } from 'immutable'; import { Row } from './row'; import { elementInViewport } from '../../utils/element_in_view'; import { Loading } from '../loading'; +import { loadingEnhancer } from '../loading_enhancer'; -export class List extends React.PureComponent { +class List extends React.PureComponent { props: { currentPage: ?Map, activeChangesetId: ?number, - pageIndex: number, - loading: boolean + pageIndex: number }; shouldComponentUpdate(nextProps: Object) { return ( - nextProps.loading !== this.props.loading || nextProps.activeChangesetId !== this.props.activeChangesetId || nextProps.currentPage !== this.props.currentPage ); @@ -29,25 +28,26 @@ export class List extends React.PureComponent { render() { return (
    - {this.props.loading - ? - :
    - {this.props.currentPage && - this.props.currentPage.get('features').map((f, k) => - - )} -
    } +
    + {this.props.currentPage && + this.props.currentPage.get('features').map((f, k) => + + )} +
); } } + +List = loadingEnhancer(List); +export { List }; diff --git a/src/components/loading_enhancer.js b/src/components/loading_enhancer.js new file mode 100644 index 00000000..c75319cc --- /dev/null +++ b/src/components/loading_enhancer.js @@ -0,0 +1,8 @@ +// @flow +import React from 'react'; +import { Loading } from './loading'; + +export function loadingEnhancer(WrappedComponent: any) { + return ({ loading, ...props }: Object) => + loading ? : ; +} diff --git a/src/utils/promise.js b/src/utils/promise.js index 5be14328..3eff8a18 100644 --- a/src/utils/promise.js +++ b/src/utils/promise.js @@ -1,6 +1,6 @@ // @flow -export type cancelablePromiseType = { promise: Promise<*>, cancel: () => any }; -export function cancelablePromise(promise: Promise<*>): cancelablePromiseType { +// export type cancelablePromiseType = { promise: Promise<*>, cancel: () => any }; +export function cancelablePromise(promise: Promise<*>) { let hasCanceled_ = false; const wrappedPromise = new Promise((resolve, reject) => { diff --git a/src/views/changesets_list.js b/src/views/changesets_list.js index 5ed8c2a1..83430467 100644 --- a/src/views/changesets_list.js +++ b/src/views/changesets_list.js @@ -14,8 +14,8 @@ import { } from '../store/changesets_page_actions'; import { List } from '../components/list'; +import { Footer } from '../components/list/footer'; import { Button } from '../components/button'; -import { PageRange } from '../components/list/page_range'; import { Dropdown } from '../components/dropdown'; import { keyboardToggleEnhancer } from '../components/keyboard_enhancer'; @@ -25,15 +25,8 @@ import { FILTER_BINDING } from '../config/bindings'; -import { PAGE_SIZE } from '../config/constants'; - import filters from '../config/filters.json'; -const RANGE = 6; - -function range(start, end) { - return Array.from(new Array(end - start)).map((k, i) => i + start); -} class ChangesetsList extends React.PureComponent { props: { location: Object, @@ -92,8 +85,7 @@ class ChangesetsList extends React.PureComponent { componentWillReceiveProps(nextProps) { const lastKeyStroke: Map = nextProps.lastKeyStroke; if (is(this.props.lastKeyStroke, lastKeyStroke)) return; - var string = lastKeyStroke.keySeq().first(); - switch (string) { + switch (lastKeyStroke.keySeq().first()) { case FILTER_BINDING.label: { this.toggleFilters(); break; @@ -111,7 +103,6 @@ class ChangesetsList extends React.PureComponent { } } } - handleFilterOrderBy = (selected: Array<*>) => { let mergedFilters; mergedFilters = this.props.filters.set('order_by', fromJS(selected)); @@ -123,17 +114,15 @@ class ChangesetsList extends React.PureComponent { this.props.getChangesetsPage(this.props.pageIndex, true); }; render() { - const base = parseInt(this.props.pageIndex / RANGE, 10) * RANGE; - const { currentPage, loading, diff, diffLoading } = this.props; - if ( - this.props.pageIndex === 0 && - currentPage && - !Number.isNaN(currentPage.get('count', 10)) - ) { - const count: number = currentPage.get('count', 10); - this.maxPageCount = Math.ceil(count / PAGE_SIZE); - } + // if ( + // this.props.pageIndex === 0 && + // currentPage && + // !Number.isNaN(currentPage.get('count', 10)) + // ) { + // const count: number = currentPage.get('count', 10); + // this.maxPageCount = Math.ceil(count / PAGE_SIZE); + // } const valueData = []; const options = filters.filter(f => f.name === 'order_by')[0].options; @@ -206,31 +195,11 @@ class ChangesetsList extends React.PureComponent { loading={loading} pageIndex={this.props.pageIndex} /> -
- - {range(base, Math.min(base + RANGE, this.maxPageCount)).map(n => - - )} - = this.maxPageCount} - pageIndex={this.props.pageIndex + 1} - active={false} - getChangesetsPage={this.props.getChangesetsPage} - /> -
+