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

Refactor loading actions #2996

Merged
merged 3 commits into from
Feb 21, 2024
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
3 changes: 0 additions & 3 deletions client/constants.js
Original file line number Diff line number Diff line change
Expand Up @@ -137,9 +137,6 @@ export const SET_SORT_PARAMS = 'SET_SORT_PARAMS';
export const SET_SEARCH_TERM = 'SET_SEARCH_TERM';
export const CLOSE_SKETCHLIST_MODAL = 'CLOSE_SKETCHLIST_MODAL';

export const START_LOADING = 'START_LOADING';
export const STOP_LOADING = 'STOP_LOADING';

export const START_SAVING_PROJECT = 'START_SAVING_PROJECT';
export const END_SAVING_PROJECT = 'END_SAVING_PROJECT';

Expand Down
2 changes: 1 addition & 1 deletion client/modules/IDE/actions/assets.js
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import apiClient from '../../../utils/apiClient';
import * as ActionTypes from '../../../constants';
import { startLoader, stopLoader } from './loader';
import { startLoader, stopLoader } from '../reducers/loading';
import { assetsActions } from '../reducers/assets';

const { setAssets, deleteAsset } = assetsActions;
Expand Down
2 changes: 1 addition & 1 deletion client/modules/IDE/actions/collections.js
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import browserHistory from '../../../browserHistory';
import apiClient from '../../../utils/apiClient';
import * as ActionTypes from '../../../constants';
import { startLoader, stopLoader } from './loader';
import { startLoader, stopLoader } from '../reducers/loading';
import { setToastText, showToast } from './toast';

const TOAST_DISPLAY_TIME_MS = 1500;
Expand Down
9 changes: 0 additions & 9 deletions client/modules/IDE/actions/loader.js

This file was deleted.

2 changes: 1 addition & 1 deletion client/modules/IDE/actions/projects.js
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import apiClient from '../../../utils/apiClient';
import * as ActionTypes from '../../../constants';
import { startLoader, stopLoader } from './loader';
import { startLoader, stopLoader } from '../reducers/loading';

// eslint-disable-next-line
export function getProjects(username) {
Expand Down
4 changes: 2 additions & 2 deletions client/modules/IDE/actions/projects.unit.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -33,9 +33,9 @@ describe('projects action creator tests', () => {
store = mockStore(initialTestState);

const expectedActions = [
{ type: ActionTypes.START_LOADING },
{ type: 'loading/startLoader' },
Copy link
Collaborator

Choose a reason for hiding this comment

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

Either of these should work:

{ type: startLoader.type },
startLoader(),

startLoader() is an action creator, so it's a function that returns the action. If you just call that function without dispatching you will get the value { type: 'loading/startLoader' }.

{ type: ActionTypes.SET_PROJECTS, projects: mockProjects },
{ type: ActionTypes.STOP_LOADING }
{ type: 'loading/stopLoader' }
];

return store
Expand Down
15 changes: 5 additions & 10 deletions client/modules/IDE/components/AddToCollectionList.jsx
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import PropTypes from 'prop-types';
import React, { useEffect, useState } from 'react';
import React, { useEffect } from 'react';
import { Helmet } from 'react-helmet';
import { useTranslation } from 'react-i18next';
import { useDispatch, useSelector } from 'react-redux';
Expand All @@ -13,6 +13,7 @@ import {
import getSortedCollections from '../selectors/collections';
import QuickAddList from './QuickAddList';
import { remSize } from '../../../theme';
import { startLoader, stopLoader } from '../reducers/loading';

export const CollectionAddSketchWrapper = styled.div`
width: ${remSize(600)};
Expand All @@ -29,20 +30,14 @@ export const QuickAddWrapper = styled.div`

const AddToCollectionList = ({ projectId }) => {
const { t } = useTranslation();

const dispatch = useDispatch();

const username = useSelector((state) => state.user.username);

const collections = useSelector(getSortedCollections);

// TODO: improve loading state
const loading = useSelector((state) => state.loading);
const [hasLoadedData, setHasLoadedData] = useState(false);
const showLoader = loading && !hasLoadedData;

useEffect(() => {
dispatch(getCollections(username)).then(() => setHasLoadedData(true));
dispatch(startLoader());
dispatch(getCollections(username)).then(() => dispatch(stopLoader()));
Copy link
Collaborator

Choose a reason for hiding this comment

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

Check out the source of the getCollections thunk. I think that it dispatches startLoader and stopLoader on its own already?

}, [dispatch, username]);

const handleCollectionAdd = (collection) => {
Expand All @@ -60,7 +55,7 @@ const AddToCollectionList = ({ projectId }) => {
}));

const getContent = () => {
if (showLoader) {
if (loading) {
return <Loader />;
} else if (collections.length === 0) {
return t('AddToCollectionList.Empty');
Expand Down
15 changes: 5 additions & 10 deletions client/modules/IDE/components/AddToCollectionSketchList.jsx
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import PropTypes from 'prop-types';
import React, { useEffect, useState } from 'react';
import React, { useEffect } from 'react';
import { Helmet } from 'react-helmet';
import { useDispatch, useSelector } from 'react-redux';
import { useTranslation } from 'react-i18next';
Expand All @@ -12,23 +12,18 @@ import {
CollectionAddSketchWrapper,
QuickAddWrapper
} from './AddToCollectionList';
import { startLoader, stopLoader } from '../reducers/loading';

const AddToCollectionSketchList = ({ collection }) => {
const { t } = useTranslation();

const dispatch = useDispatch();

const username = useSelector((state) => state.user.username);

const sketches = useSelector(getSortedSketches);

// TODO: improve loading state
const loading = useSelector((state) => state.loading);
const [hasLoadedData, setHasLoadedData] = useState(false);
const showLoader = loading && !hasLoadedData;

useEffect(() => {
dispatch(getProjects(username)).then(() => setHasLoadedData(true));
dispatch(startLoader());
dispatch(getProjects(username)).then(() => dispatch(stopLoader()));
}, [dispatch, username]);

const handleCollectionAdd = (sketch) => {
Expand All @@ -48,7 +43,7 @@ const AddToCollectionSketchList = ({ collection }) => {
}));

const getContent = () => {
if (showLoader) {
if (loading) {
return <Loader />;
} else if (sketches.length === 0) {
// TODO: shouldn't it be NoSketches? -Linda
Expand Down
21 changes: 10 additions & 11 deletions client/modules/IDE/reducers/loading.js
Original file line number Diff line number Diff line change
@@ -1,14 +1,13 @@
import * as ActionTypes from '../../../constants';
import { createSlice } from '@reduxjs/toolkit';

const loading = (state = false, action) => {
switch (action.type) {
case ActionTypes.START_LOADING:
return true;
case ActionTypes.STOP_LOADING:
return false;
default:
return state;
const loadingSlice = createSlice({
name: 'loading',
initialState: false,
reducers: {
startLoader: () => true,
stopLoader: () => false
}
};
});

export default loading;
export const { startLoader, stopLoader } = loadingSlice.actions;
export default loadingSlice.reducer;
12 changes: 8 additions & 4 deletions client/modules/Legal/pages/Legal.jsx
Copy link
Collaborator

@lindapaiste lindapaiste Feb 6, 2024

Choose a reason for hiding this comment

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

This one is subjective. Personally I would use the local component state. The policy loading state is not used in any other component so I don't think that it needs to be in a global Redux state.

Original file line number Diff line number Diff line change
Expand Up @@ -4,12 +4,14 @@ import React, { useEffect, useState } from 'react';
import Helmet from 'react-helmet';
import { useTranslation } from 'react-i18next';
import styled from 'styled-components';
import { useDispatch, useSelector } from 'react-redux';
import RouterTab from '../../../common/RouterTab';
import RootPage from '../../../components/RootPage';
import { remSize } from '../../../theme';
import Loader from '../../App/components/loader';
import Nav from '../../IDE/components/Header/Nav';
import PolicyContainer from '../components/PolicyContainer';
import { startLoader, stopLoader } from '../../IDE/reducers/loading';

const StyledTabList = styled.nav`
display: flex;
Expand All @@ -24,15 +26,17 @@ const StyledTabList = styled.nav`

function Legal({ policyFile, title }) {
const { t } = useTranslation();
const [isLoading, setIsLoading] = useState(true);
const loading = useSelector((state) => state.loading);
const dispatch = useDispatch();
const [policy, setPolicy] = useState('');

useEffect(() => {
dispatch(startLoader());
axios.get(policyFile).then((response) => {
dispatch(stopLoader());
setPolicy(response.data);
setIsLoading(false);
});
}, [policyFile]);
}, [dispatch, policyFile]);

return (
<RootPage>
Expand All @@ -51,7 +55,7 @@ function Legal({ policyFile, title }) {
</ul>
</StyledTabList>
<PolicyContainer policy={policy} />
{isLoading && <Loader />}
{loading && <Loader />}
</RootPage>
);
}
Expand Down
Loading