Moved token and getUserQueues selectors to prevent useless side effects

[maaverik]

No description provided.

[maaverik]

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.

[maaverik]

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.

[maaverik]

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.

[mradenovic]

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).

[maaverik]

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?

[mradenovic]

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.

[maaverik]

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.

[maaverik]

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.

[daltonfury42]

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?

[maaverik]

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?

[mradenovic]

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.

[maaverik]

I moved the getUserQueues dispatch back to Layout and changed the popup to only show up from UserQueues. There's still a problem though, whenever I route back to the HompePage, I still see the /queues network call 😕. I couldn't figure out where I was going wrong. I tried removing all the dependencies of the useEffect() in Layout and I still saw the network call. Let me know if you have some ideas @daltonfury42 @mradenovic.

[maaverik]

I think I got the problem. The logo and the Home button work by directly changing the route and don't go through the react-router. I think this causes the whole app to re-render and cause the UseEffect to trigger. If this is really the reason, we'll need to chnage it to go through the router if we want to get rid of that fetch.

[maaverik]

Things should be good now.

[mradenovic]

Things should be good now.

Check #545. I identified couple of causes for reload.

[daltonfury42]

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.

Understood.

[daltonfury42]

lets create a util function onLoadbyId:

/** 
 * Execute a callback as soon as an element is available on the DOM.
 *
 * id - element id to wait on
 * callback - callback to execute as soon as the element becomes available. The 
 * element is passed to the callback and it is triggered.
function onLoadById(id, callback) {
  var checkAndExecute = setInterval(function() {
     const element = document.getElementById(id);
     if (element) {
        callback(element);
        clearInterval(checkAndExecute);
     }
  }, 100);
}

then just use it here: onLoadById('target_how_it_works', e =>smoothScrollTo(e))

[daltonfury42]

The above was inspired by this, you can check if there are better ways to do this, though.

[daltonfury42]

I think as a follow up PR, we can have a single function in scrollingOperations.js:

/**
 * Smoothly scroll and take the user to a section of the page.
 * 
 * targetElement - id of the element to scroll to
 * page - Optional. The page to go to, before scrolling to the section, 
 * in case user is not currently on the specified page.
 * history - react router history object, required if page is specified
 **/ 
smoothScrollToSection(targetElement, page, history) {
  ...
}

Then we could export wrappers on top of this, smoothScrollToHomePage(id, history) and smoothScrollWithinCurrentPage(id). What do you think?

[maaverik]

Then we could export wrappers on top of this, smoothScrollToHomePage(id, history) and smoothScrollWithinCurrentPage(id). What do you think?

Sounds good