Skip to content
This repository has been archived by the owner on Jun 3, 2024. It is now read-only.

QMAPS-2033 add "add to home" infobox on mobile after the 5th visit #1161

Draft
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

xem
Copy link
Contributor

@xem xem commented Jul 21, 2021

Description

add "add to home" infobox on mobile after the 5th visit
TODO: use com center link when it's ready

Screenshots

addtohome

@xem xem marked this pull request as draft July 21, 2021 09:11
};

const visits = parseInt(get('visits') || 0) + 1;
set('visits', visits);
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a good example of a side effect that we want to control more and safely separate from the component rendering.

If we put it here in the render function, we will increment visits each time this component is re-rendered, which can happen at any moment in the app not related to a single hit.
You can already see it in these cases:

  • when the window is resized and the device is changed from desktop to mobile (as it's rendered as a child of a DeviceContext.Provider)
  • after the box is displayed and we call setClosed(true)

To ensure this increment happens only on mounting the component, you could put it inside a useEffect(…, [ ]).
But this still means if for some reason we unmount and remount this component during a user session, it will be counted twice.
So the safer approach IMO is to update this counter outside this component, where we are 100% sure we'll execute code only once per session. For example, as a sideEffect of RootComponent or in app_panel. The get can still happen here as it doesn't modify anything.

@bbecquet
Copy link
Contributor

bbecquet commented Aug 5, 2021

The <Notification> component has been removed as part of #1172. It was to avoid dead code, but I forgot it was used by this PR.
I rebased it with a commit restoring the component.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants