1618 add view count to how to and research#2086
Conversation
e7f8428 to
2681a8f
Compare
acf8521 to
4f74c9d
Compare
4f74c9d to
646ba53
Compare
Passing run #2902 ↗︎Details:
This comment has been generated by cypress-bot as a result of this project's GitHub integration settings. |
|||||||||||||||
|
@davehakkens the link for the icon download wasn't working so added the star icon in the meantime, I can add the correct icon whenever you have time and can upload it. The PR also fixed some bugs I noticed with the download counter which now should be all sorted, state wasn't being updated properly. I also ended up sticking with using |
|
Visit the preview URL for this PR (updated for commit 765a49c): https://onearmy-next--pr2086-1618-add-view-count-zsl9v0ls.web.app (expires Mon, 20 Mar 2023 13:54:28 GMT) 🔥 via Firebase Hosting GitHub Action 🌎 Sign: 6d65e4f8fee2f6ab2da0c1c3b85b8797d66afa59 |
|
thanks @AlfonsoGhislieri Could you wrap it in our AuthWrapper so it's only available for |
|
|
||
| export const ViewsCounter = (props: IProps) => { | ||
| return ( | ||
| <> |
There was a problem hiding this comment.
We don't need a fragment here. It is already returning a single element which is Button.
| sx={{ | ||
| backgroundColor: 'transparent', | ||
| fontSize: 2, | ||
| ml: 2, | ||
| background: 'softyellow', | ||
| borderColor: 'softyellow', | ||
| opacity: 1, | ||
| pointerEvents: 'none', | ||
| cursor: 'none', | ||
| }} |
There was a problem hiding this comment.
Let's not add more styling than necessary, I would suggest using the base Button styling. Since the background color is slightly different we can probably add it as a variant but things like font-sze, opacity, pointer-events and cursor should be left to the Button component to decide.
| useEffect(() => { | ||
| setFileDownloadCount(howto.total_downloads) | ||
| setViewCount(howto.total_views) | ||
| incrementViewCount() | ||
| }, [howto._id]) |
There was a problem hiding this comment.
Let's not use an effect to initialize/re-initialize state. This is considered an undesirable pattern see more:
https://beta.reactjs.org/learn/you-might-not-need-an-effect#resetting-all-state-when-a-prop-changes
| interface IState { | ||
| fileDownloadCount: number | undefined | ||
| } | ||
| const HowtoDescription: React.FC<IProps> = ({ |
There was a problem hiding this comment.
React.FC is generally discouraged, see the Why is React.FC discouraged section here:
https://react-typescript-cheatsheet.netlify.app/docs/basic/getting-started/function_components
| const iconFlexDirection = | ||
| emStringToPx(theme.breakpoints[0]) > window.innerWidth ? 'column' : 'row' |
There was a problem hiding this comment.
I think this can be moved to outside the component without any issue, move it back if you find any issue though.
| mb="2" | ||
| </Box> | ||
| )} | ||
| <Box sx={{ flexGrow: 1 }}> |
There was a problem hiding this comment.
Instead of using flex-grow: 1 here, let's remove justify-content: space-between from the parent of these buttons/link and add a gap as well. We can then remove left margin from our buttons as well.
| useEffect(() => { | ||
| setViewCount(research.total_views) | ||
| incrementViewCount() | ||
| }, [research._id]) |
There was a problem hiding this comment.
Please check the useEffect comment within the HowToDescription component.
| /> | ||
| </Box> | ||
| )} | ||
| <Box style={{ flexGrow: 1 }}> |
There was a problem hiding this comment.
Please check the flex-grow comment within HowToDescription component.
|
@asheerrizvi thanks for the feedback! Made all the changes and also converted the |
| useEffect(() => { | ||
| incrementViewCount() | ||
| }, [research._id]) |
There was a problem hiding this comment.
I think if you can do a lazy initialization for useState and not be in need of this effect:
https://kentcdodds.com/blog/use-state-lazy-initialization-and-function-updates
There was a problem hiding this comment.
The issue is as far as I know you can't pass an asynchronous function to do a lazy initialization for useState it'll set state to a promise rather than the return value of the function. I had already tried to do this initially and had to revert to useEffect for that reason.
There was a problem hiding this comment.
Ok, I am approving this. Can we create a new issue for removing this effect and work on it separately?
| useEffect(() => { | ||
| incrementViewCount() | ||
| }, [howto._id]) |
There was a problem hiding this comment.
See comment on ResearchDescription component please:
https://kentcdodds.com/blog/use-state-lazy-initialization-and-function-updates
|
🎉 This PR is included in version 1.38.0 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
|
Thanks @AlfonsoGhislieri Its on live now. |
Great! Let me know if there’s any issues or anything you’d like changed :) |
|
Will do. One question @AlfonsoGhislieri to make sure I got it correct: Whats the formula you used to calculate a view? |
|
A view gets added each time a resource or howto page is opened or refreshed. However, to limit bloating of views each time you view a page the id is stored in Session storage and this is checked every time you load a page. This all gets reset after the session ends (when the tab or browser gets closed). So if you’ll view the page again after that a new view will be added. Hope that makes sense! |
|
makes sense! |
|
Right now it counts all views! This could be changed if you want? |
|
perfect! Should count all, but only show to beta-testers |
|
Are you sure about this @AlfonsoGhislieri |
|
Damn well this is quite confusing, it definitely works like intended when I run it locally. I just tested it multiple times to make sure, the views get incremented correctly when you open the page even just as a normal user (either loading the url or by clicking on the howto/research directly on the main page). But you are right this clearly looks something is not working like it should in the production environment, I have no clue as to why this is the case though since everything appears to be working normally when I test it. Is there any chance you could make my account a beta-tester so I can have a look on the production side if I can figure something out? |
|
Cryptiks on the platform Ah so the views are actually being correctly incremented in the backend but just not rendering correctly? Even more confused hmm... Happy to have a look at the firebase as well, might help understand what's up |





PR Checklist
PR Type
Description
Implements a simple way for views to be tracked, using sessionstorage to limit views being inflated by refreshing page.
Git Issues
Closes #
Add view count to How-to and Research #1618
Screenshots/Videos
If useful, provide screenshot or capture to highlight main changes
What happens next?
Thanks for the contribution! We try to make sure all PRs are reviewed ahead of a monthly dev call (first Monday of the month, open to all!).
If the PR is working as intended it'll be merged and included in the next platform release, if not changes will be requested and re-reviewed once updated.
If you need more immediate feedback you can try reaching out on Discord in the Community Platform
developmentchannel.