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

Moved token and getUserQueues selectors to prevent useless side effects #556

Merged
merged 9 commits into from
Feb 28, 2021
Merged

Moved token and getUserQueues selectors to prevent useless side effects #556

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
ed068be
Moved token selector to tokenlist
maaverik Feb 23, 2021
6dcc541
Moved getUserQueues dispatch to LandingPage to prevent unneeded popup…
maaverik Feb 23, 2021
ecead5d
Removed console.log
maaverik Feb 23, 2021
c6fa0cc
Moved getUserQueues back to Layout and changes the popup behaviour
maaverik Feb 27, 2021
75d4080
Changed anchor tag redirects to use react router history
maaverik Feb 27, 2021
e0e554d
Add comments to layout
daltonfury42 Feb 28, 2021
26e9886
Fix code style issues with Prettier
lint-action Feb 28, 2021
c2c4ffa
Made utility function for delayed scrolling
maaverik Feb 28, 2021
c530733
Merge branch 'moving-logic' of https://github.com/SimplQ/simplQ-front…
maaverik Feb 28, 2021
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
16 changes: 2 additions & 14 deletions simplq/src/components/Layout/Layout.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,24 +1,12 @@
import React, { useEffect } from 'react';
import React from 'react';
import { useAuth0 } from '@auth0/auth0-react';
import Footer from 'components/common/Footer';
import Loading from 'components/common/Loading/Loading';
import { useGetUserQueues } from 'store/asyncActions';
import { useDispatch } from 'react-redux';
import Routes from './Routes';
import styles from './Layout.module.scss';

function Layout() {
const { isLoading, isAuthenticated } = useAuth0();
const dispatch = useDispatch();
const getUserQueues = useGetUserQueues();

useEffect(() => {
// TODO: Server still returns queues for anonymous users
// Dispatch action only if auth is loaded
if (isLoading === false) {
dispatch(getUserQueues());
}
}, [isLoading, isAuthenticated, getUserQueues, dispatch]);
const { isLoading } = useAuth0();

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Since Layout is used in all pages, the active queues were being fetched and the popup was showing up if we directly load any of the other pages which is not needed.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Maybe we would want to move this to MyQueues instead of LandingPage, since it's only used in the homePage right now, I left it in LandingPage.

Copy link
Contributor

Choose a reason for hiding this comment

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

The idea here was to move all requests further up in the component tree, so the data is ready no matter what URL the user is coming from. It is a step closer to a single-page app. Currently, the home page will send unnecessary requests to the server on almost every render (as described in #545).

This can be eventually converted into PWA, so everything is loaded only once, and updated as needed.

Since re-render is cheaper than re-fetch, I would put the effort in fixing the behavior (e.g. remove the popup, update store on successful requests).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That's a good strategy, but I'm still wondering if that should apply to requests like this one which only make sense in one route and only for admins. We would definitely have more queue members than admins, so do we really want to make the getUserQueues() request trigger for users who just want to join a queue and keep tabs on the status?

Also since this request is only needed in the homepage, do we have to worry about unnecessary re-fetches? On page refresh, it triggers each time, but we would want that for consistency's sake, right?

Copy link
Contributor

Choose a reason for hiding this comment

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

We could add MyQueues to the admin panel, so an admin can quickly switch queues from the admin panel without going to the homepage. And if a user is not the admin, there will be nothing to fetch.

Also since this request is only needed in the homepage, do we have to worry about unnecessary re-fetches?

Currently, if the user switches between home and admin, the fetch is triggered every time.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Currently, if the user switches between home and admin, the fetch is triggered every time.

Oh yeah, I missed this. Leaving it in the layout is a good idea to prevent this. Good catch. I'll revert this change.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm thinking of changing the popup behavior by removing the extraReducer in AppSlice and adding a popup trigger inside the MyQueues component. That way, the popup only shows up when needed.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Currently, if the user switches between home and admin, the fetch is triggered every time.

Is this desirable? I think we should fetch every time home page is rendered. So can we dispatch at root of home page?

Copy link
Collaborator Author

@maaverik maaverik Feb 27, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Is this desirable? I think we should fetch every time home page is rendered.

It would be needed for the admin to see his newly created queue in the list. I was initially thinking of leaving the dispatch in the Layout (so fetch only happens once) and manually adding to the queues state variable for all newly created queues after the fetch, but it's simpler and cleaner to re-fetch on every home page render. It isn't too big of a cost in terms of data returned and if it ever looks like that could be an issue, we can limit the number of queues an admin can own at a time.

So can we dispatch at root of home page?

Do you mean change it from the way it's written in this PR? To where?

Copy link
Contributor

Choose a reason for hiding this comment

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

No need to fetch every time the page is rendered. You fetch once the app is loaded. Then on successful add or delete, just update the state.

return (
<div className={styles['box']}>
Expand Down
6 changes: 2 additions & 4 deletions simplq/src/components/pages/Admin/AdminPage.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
import React, { useState, useEffect, useCallback } from 'react';
import { useAuth0 } from '@auth0/auth0-react';
import { useDispatch, useSelector } from 'react-redux';
import { selectTokens, selectQueueName } from 'store/selectedQueue';
import { selectQueueName } from 'store/selectedQueue';
import Ribbon from 'components/common/Ribbon';
import Tour from 'components/common/Tour';
import { useGetSelectedQueue } from 'store/asyncActions';
Expand All @@ -19,14 +19,12 @@ let timeoutId;
export default (props) => {
const queueId = props.match.params.queueId;
const queueName = useSelector(selectQueueName);
const tokens = useSelector(selectTokens);
const description = queueName && 'Ready to share';
const dispatch = useDispatch();
const getSelectedQueue = useGetSelectedQueue();

const [toursteps, setToursteps] = useState(getToursteps(window.innerHeight));
const { isAuthenticated } = useAuth0();

const update = useCallback(() => {
clearTimeout(timeoutId);
dispatch(getSelectedQueue({ queueId }));
Expand Down Expand Up @@ -58,7 +56,7 @@ export default (props) => {
/>
)}
<div className={styles['main-body']}>
<TokenList tokens={tokens} queueId={queueId} />
<TokenList queueId={queueId} />
<SidePanel queueId={queueId} />
</div>
</div>
Expand Down
9 changes: 6 additions & 3 deletions simplq/src/components/pages/Admin/TokenList.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,10 +1,13 @@
import React from 'react';
import Loading from 'components/common/Loading/Loading';
import { useSelector } from 'react-redux';
import { selectTokens } from 'store/selectedQueue';
import Token from './Token';
import styles from './admin.module.scss';

function TokenList({ tokens }) {
const Dispaly = () =>
function TokenList() {
const tokens = useSelector(selectTokens);
const ListContent = () =>
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Using this selector in admin page was resulting in an unnecessary admin page code re-render, so moved here since only TokenList should be affected by token slice changes.

tokens.length === 0 ? (
<p>Your queue has been created and is currently empty. Waiting for people to join...</p>
) : (
Expand All @@ -14,7 +17,7 @@ function TokenList({ tokens }) {
return (
<Loading isLoading={tokens === undefined}>
<div className={styles['token-list']}>
<Dispaly />
<ListContent />
</div>
</Loading>
);
Expand Down
17 changes: 15 additions & 2 deletions simplq/src/components/pages/Home/LandingPage.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,16 +1,29 @@
import React from 'react';
import React, { useEffect } from 'react';
import { useAuth0 } from '@auth0/auth0-react';
import Header from 'components/common/Header';
import QueueForm from 'components/common/CreateJoinForm';
import { useGetUserQueues } from 'store/asyncActions';
import { useDispatch } from 'react-redux';
import styles from './Home.module.scss';
import MyQueues from './MyQueues';

export default () => {
let subtitle = 'A long overdue alternative to physical queues';
const { user, isAuthenticated } = useAuth0();
const { user, isAuthenticated, isLoading } = useAuth0();
if (isAuthenticated) {
subtitle = `Hi ${user.name}, welcome back!`;
}
const dispatch = useDispatch();
const getUserQueues = useGetUserQueues();

useEffect(() => {
// TODO: Server still returns queues for anonymous users
// Dispatch action only if auth is loaded
daltonfury42 marked this conversation as resolved.
Show resolved Hide resolved
if (isLoading === false) {
dispatch(getUserQueues());
daltonfury42 marked this conversation as resolved.
Show resolved Hide resolved
}
}, [isLoading, isAuthenticated, getUserQueues, dispatch]);

return (
<div id="target_top" className={styles['landing-page']}>
<div data-aos="zoom-in">
Expand Down