Skip to content

Commit

Permalink
Change loading indicator UI/UX to rely on fetching state
Browse files Browse the repository at this point in the history
In the previous implementation, show a loading indicator infinitely when
API server response has no items. So, I improve this.
  • Loading branch information
taehwanno committed Jan 21, 2018
1 parent e8cdbe0 commit 78a16bd
Show file tree
Hide file tree
Showing 11 changed files with 119 additions and 6 deletions.
4 changes: 4 additions & 0 deletions app/components/HackerNewsList/HackerNewsList.scss
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,10 @@
}
}

.HackerNewsList__noti {
text-align: center;
}

.HackerNewsList__item {
margin: 30px 0;
}
Expand Down
5 changes: 5 additions & 0 deletions app/components/HackerNewsList/HackerNewsList.spec.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -40,4 +40,9 @@ describe('<HackerNewsList />', () => {
const wrapper = shallow(<HackerNewsList feeds={feeds} />);
expect(wrapper).toMatchSnapshot();
});

it('should have .HackerNewsList__noti when have no items', () => {
const wrapper = shallow(<HackerNewsList feeds={Immutable.List()} isFetching={false} />);
expect(wrapper.find('.HackerNewsList__noti')).toHaveLength(1);
});
});
4 changes: 3 additions & 1 deletion app/components/HackerNewsList/HackerNewsList.stories.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,9 @@ stories
{story()}
</MemoryRouter>
))
.add('default', () => (
.add('fetching data', () => <HackerNewsList isFetching />)
.add('no data', () => <HackerNewsList feeds={Immutable.List()} isFetching={false} />)
.add('with data', () => (
<HackerNewsList
feeds={Immutable.fromJS([
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ exports[`<HackerNewsList /> should match snapshot when render default 1`] = `
className="HackerNewsList"
>
<LoadingIndicator
active={true}
active={false}
style={
Object {
"left": "calc(50% - 24px)",
Expand All @@ -14,6 +14,11 @@ exports[`<HackerNewsList /> should match snapshot when render default 1`] = `
}
}
/>
<p
className="HackerNewsList__noti"
>
There are no items to show.
</p>
</ul>
`;

Expand Down
11 changes: 8 additions & 3 deletions app/components/HackerNewsList/index.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -9,20 +9,25 @@ import './HackerNewsList.scss';

const propTypes = {
feeds: PropTypes.object, // eslint-disable-line react/forbid-prop-types
isFetching: PropTypes.bool,
};

const defaultProps = {
feeds: Immutable.List(),
isFetching: false,
};

function HackerNewsList({ feeds }) {
function HackerNewsList({ feeds, isFetching }) {
const haveNoItems = !isFetching && feeds.size === 0;

return (
<ul className="HackerNewsList">
<LoadingIndicator
active={!feeds.size}
active={isFetching}
style={{ position: 'absolute', left: 'calc(50% - 24px)', marginTop: '10px' }}
/>
{feeds.map((feed, index) => (
{haveNoItems && <p className="HackerNewsList__noti">There are no items to show.</p>}
{!isFetching && feeds.map((feed, index) => (
<li className="HackerNewsList__item" key={feed.get('id')}>
<span className="HackerNewsList__index">{index + 1}</span>
<HackerNewsListItem {...feed.toJS()} />
Expand Down
3 changes: 2 additions & 1 deletion app/containers/HackerNewsListContainer.jsx
Original file line number Diff line number Diff line change
@@ -1,10 +1,11 @@
import { connect } from 'react-redux';

import HackerNewsList from 'components/HackerNewsList';
import { getFeeds } from 'store/selectors';
import { getFeeds, getIsFetching } from 'store/selectors';

const mapStateToProps = (state, props) => ({
feeds: getFeeds(state, props),
isFetching: getIsFetching(state),
});

export default connect(mapStateToProps)(HackerNewsList);
62 changes: 62 additions & 0 deletions app/store/__tests__/reducers/isFetching.spec.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,62 @@
import * as ACTIONS from 'store/actionTypes';
import isFetching from 'store/isFetching';

describe('isFetching reducer', () => {
it('should return the initial state', () => {
expect(isFetching(undefined, {})).toBe(false);
});

it('should handle HACKER_COMMENTS_FETCH_REQUEST', () => {
expect(isFetching(false, {
type: ACTIONS.HACKER_COMMENTS_FETCH_REQUEST,
})).toBe(true);
});

it('should handle HACKER_COMMENTS_FETCH_SUCCESS', () => {
expect(isFetching(true, {
type: ACTIONS.HACKER_COMMENTS_FETCH_SUCCESS,
})).toBe(false);
});

it('should handle HACKER_COMMENTS_FETCH_FAILURE', () => {
expect(isFetching(true, {
type: ACTIONS.HACKER_COMMENTS_FETCH_FAILURE,
})).toBe(false);
});

it('should handle HACKER_NEWS_FETCH_REQUEST', () => {
expect(isFetching(false, {
type: ACTIONS.HACKER_NEWS_FETCH_REQUEST,
})).toBe(true);
});

it('should handle HACKER_NEWS_FETCH_SUCCESS', () => {
expect(isFetching(true, {
type: ACTIONS.HACKER_NEWS_FETCH_SUCCESS,
})).toBe(false);
});

it('should handle HACKER_NEWS_FETCH_FAILURE', () => {
expect(isFetching(true, {
type: ACTIONS.HACKER_NEWS_FETCH_FAILURE,
})).toBe(false);
});

it('should handle HACKER_USER_FETCH_REQUEST', () => {
expect(isFetching(false, {
type: ACTIONS.HACKER_USER_FETCH_REQUEST,
})).toBe(true);
});

it('should handle HACKER_USER_FETCH_SUCCESS', () => {
expect(isFetching(true, {
type: ACTIONS.HACKER_USER_FETCH_SUCCESS,
})).toBe(false);
});

it('should handle HACKER_USER_FETCH_FAILURE', () => {
expect(isFetching(true, {
type: ACTIONS.HACKER_USER_FETCH_FAILURE,
})).toBe(false);
});
});
5 changes: 5 additions & 0 deletions app/store/__tests__/selectors.spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ describe('selectors', () => {
posts: Immutable.Map(),
},
currentPage: 1,
isFetching: false,
items: {
ask: {},
jobs: {},
Expand All @@ -27,6 +28,10 @@ describe('selectors', () => {
expect(selectors.getCurrentPage(state)).toBe(state.get('currentPage'));
});

it('should select isFetching', () => {
expect(selectors.getIsFetching(state)).toBe(state.get('isFetching'));
});

it('should select items', () => {
expect(selectors.getItems(state)).toEqual(state.get('items'));
});
Expand Down
21 changes: 21 additions & 0 deletions app/store/isFetching/index.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
import * as ACTIONS from '../actionTypes';

function itemsReducer(state = false, action) {
switch (action.type) {
case ACTIONS.HACKER_COMMENTS_FETCH_REQUEST:
case ACTIONS.HACKER_NEWS_FETCH_REQUEST:
case ACTIONS.HACKER_USER_FETCH_REQUEST:
return true;
case ACTIONS.HACKER_COMMENTS_FETCH_SUCCESS:
case ACTIONS.HACKER_COMMENTS_FETCH_FAILURE:
case ACTIONS.HACKER_NEWS_FETCH_SUCCESS:
case ACTIONS.HACKER_NEWS_FETCH_FAILURE:
case ACTIONS.HACKER_USER_FETCH_SUCCESS:
case ACTIONS.HACKER_USER_FETCH_FAILURE:
return false;
default:
return state;
}
}

export default itemsReducer;
2 changes: 2 additions & 0 deletions app/store/rootReducer.js
Original file line number Diff line number Diff line change
Expand Up @@ -4,13 +4,15 @@ import { routerReducer } from 'react-router-redux';
import byId from './byId';
import comments from './comments';
import currentPage from './currentPage';
import isFetching from './isFetching';
import items from './items';
import user from './user';

const rootReducer = combineReducers({
byId,
comments,
currentPage,
isFetching,
items,
user,
router: routerReducer,
Expand Down
1 change: 1 addition & 0 deletions app/store/selectors.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ import { createSelector } from 'reselect';

export const getById = state => state.get('byId');
export const getCurrentPage = state => state.get('currentPage');
export const getIsFetching = state => state.get('isFetching');
export const getItems = state => state.get('items');
export const getUser = state => state.get('user');

Expand Down

0 comments on commit 78a16bd

Please sign in to comment.