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.

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

Show loading animation, when data fetching is in progress #42

Merged
merged 3 commits into from
Jan 11, 2018

Conversation

TheLukaszNs
Copy link

Fast Summary

This PR fixes #40 (at least loading indicator will be now shown), and adds Spinkit:

Added fancy loading indicator to show user that something is going on.
@tdevinda
Copy link
Member

The concept seems lucrative. But the content is the problem for me.
This animation has UI issues.

  1. Colors have been added which clash with both the theme and the target audience.
  2. The wording 'Firebase' should never be inserted here. It is a technical term. The app is targetted for rural low tech savvy users.

Shall we do these changes and retry?

Thanks for your enthusiasm, its a really valuable feature.

@nushydude
Copy link
Contributor

React Native Spinkit should have been used. It's pretty smooth and customisable.

@TheLukaszNs
Copy link
Author

@nushydude react-native-spinkit is used as you commented on #40
@tdevinda we can always change hardcoded string or numbers to classes or something, but I don't see 'firebase' here - it only shows loading indicator in the centre of screen.

@TheLukaszNs TheLukaszNs reopened this Jan 10, 2018
@@ -3,11 +3,17 @@ import { FETCH_DATA } from './types';
import Api from '../api/Api';

export const actionFetchData = () => (dispatch) => {
dispatch({
type: FETCH_DATA,
Copy link
Contributor

Choose a reason for hiding this comment

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

Create a new action type FETCHING_DATA and dispatch it with no payload (i.e. dispatch({ type: FETCHING_DATA }))

Better keep the shape of all actions to only have a type and a payload (& meta if required).

type: FETCH_DATA,
payload: [],
isFetching: true,
});
Api.getCombinedData(Api.getCategories, Api.getSites, Api.getThumbnail)
.then((data) => {
dispatch({
type: FETCH_DATA,
Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps we should rename this action type to FETCHED_DATA .

@@ -1,13 +1,16 @@
import { FETCH_DATA } from './../actions/types';

const InitialState = [];
const InitialState = {
isFetching: true,
Copy link
Contributor

Choose a reason for hiding this comment

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

initial state should be

{ isFetching: false, categories: [] }

@@ -35,9 +37,19 @@ class Categories extends Component {

renderCategories() {
const { navigate } = this.props.navigation;
const { categories, isFetching } = this.props.categoryData;
if (isFetching) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I would personally move the loader out of the renderCategories and move into render() function,

Something like

{ isFetching && <Loading /> }
{ !isFetching && this.renderCategories() } 

You can define a functional component called Loading like this and just use it as above.

const Loading = () => <Spinner
          type="FoldingCube"
           color="#3F51B5"
           size={100}
           style={{ marginTop: '50%' }}
         />;

If you'd like to keep the code even cleaner, create a new component called Categories (a different file) and just use and move the rendering logic into its render method.

You can extract and expose the required props from mapStateToProps. Would make things look cleaner.

const mapStateToProps = ({ data: { categories, isFetching }) => ({
  categories,
  isFetching,
});

@TheLukaszNs
Copy link
Author

@nushydude check it now, maybe there is something more I can fix?

});
case START_FETCHING:
return Object.assign({}, state, {
categories: [],
Copy link
Contributor

Choose a reason for hiding this comment

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

  1. No need to set categories to []
  2. See if you can get object spread operator to work. It would be cleaner if we could just user
return { ...state, categories: [], isFeching: true } 

for example, instead of Object.assign which is old school.

size={loaderStyle.size}
style={{ marginTop: '50%' }}
/>
);
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 are using the loaderStyle object, you should be able to use

<Spinner {...loaderStyle} style={{ marginTop: '50%' }} />

I would define style as an object, but define the options as consts.

import { Stylesheet } from 'react-native';

const TYPE = 'FolderingCube';
const COLOR = '#3F51B5';
const SIZE = 100;

const styles = Stylesheet.create({
  loader: { marginTop: '50%' },
}};

const Loading = () => (
  <Spinner
     type={TYPE}
     color={COLOR}
     size={SIZE}
     style={styles.loader}
   />
 );

Copy link
Contributor

@nushydude nushydude left a comment

Choose a reason for hiding this comment

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

I've left some comments. You can give it a shot if you are up for it. I will just approve the changes for now.

@agentmilindu agentmilindu merged commit fe3635b into CodeLanka:develop Jan 11, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants