T244548 Set up preview counting for KaiOS app #286
Conversation
| return | ||
| } | ||
|
|
||
| const logInukaPageView = () => { |
There was a problem hiding this comment.
We should probably update the name of this function :)
There was a problem hiding this comment.
Yes omg !!! Thanks @neilpquinn
src/hooks/useArticle.js
Outdated
| @@ -3,7 +3,14 @@ import { useI18n } from 'hooks' | |||
| import { getArticle, getArticleMediaList, getSuggestedArticles } from 'api' | |||
| import { canonicalizeTitle } from 'utils' | |||
|
|
|||
| let currentArticle | |||
There was a problem hiding this comment.
Adding global state like that is not great. It took me a while to figure out the whole logic here. It's clever, and it works, but I would like us to explore more conventional (or "boring") options.
One such option is to give ArticlePreview the required info about the source on open
showArticlePreview({ title, lang, dir, <info about the current article> })
There was a problem hiding this comment.
I start with the conventional options, but then it's really bad =( but anyway, let me try again with it.
There was a problem hiding this comment.
indeed, yes, useArticleLinksNavigation has galleryItems that also come from the source, we can have everything as the source, so it can pass to showArticlePreview
| return | ||
| } | ||
|
|
||
| if (page === undefined || source === undefined) { |
There was a problem hiding this comment.
When is this called without page or source?
There was a problem hiding this comment.
Just in case useArticle() is being called before the useArticle( x, y ), to make sure it returns empty {} instead of error.
There was a problem hiding this comment.
Is this still needed?
|
Ready for review again @stephanebisson |
| return | ||
| } | ||
|
|
||
| if (page === undefined || source === undefined) { |
There was a problem hiding this comment.
Is this still needed?
| useLayoutEffect(() => { | ||
| if (!page) { | ||
| return | ||
| } |
There was a problem hiding this comment.
so this is still needed here, the summary is a state and start with null until the api request is done
There was a problem hiding this comment.
Why useLayoutEffect specifically instead of useEffect?
Layout effect is blocking and should only be used to prevent flickering on visual effects.
Agree, I see the benefit of this solution now! it's easier to track and predict all the properties. |
Phabricator Link: https://phabricator.wikimedia.org/T244548
Solution
Similar to
useTrackingcode, reuse thesentEventutil for the data sendingNote
Schema url : https://meta.wikimedia.org/wiki/Schema:VirtualPageView