Skip to content

feat(site): add user activity on template insights #10013

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.

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

Merged
merged 2 commits into from
Oct 4, 2023

Conversation

BrunoQuaresma
Copy link
Collaborator

Close #9497

Screen.Recording.2023-10-03.at.10.07.32.mov

@BrunoQuaresma BrunoQuaresma requested a review from a team October 3, 2023 13:13
@BrunoQuaresma BrunoQuaresma self-assigned this Oct 3, 2023
@BrunoQuaresma BrunoQuaresma requested review from Kira-Pilot and removed request for a team October 3, 2023 13:13
@aslilac
Copy link
Member

aslilac commented Oct 3, 2023

how is colin twice as active as anyone else? 😂

Copy link
Member

@aslilac aslilac left a comment

Choose a reason for hiding this comment

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

idk if error handling is already done somewhere else, and just not showing in the diff, but we should definitely check for errors

</PanelTitle>
</PanelHeader>
<PanelContent>
{!data && <Loader sx={{ height: "100%" }} />}
Copy link
Member

Choose a reason for hiding this comment

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

I don't like checking just data for loading. we do this in a lot of places, and it ends up with lots of places that show spinners instead of error messages.

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 usually handle errors when they are expected like form errors, or possible user actions that can cause an "invalid" state. IMO handling all the errors caused by async can be too much and idk if that adds too much value, to be honest, but I'm open to that.

Maybe could be interesting to start a thread on dev about handling errors in the UI and see if we can come up with a more solid strategy.

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 also going to put some thoughts on that 🤔. Can we get this work merged and I open the PR to handle the error for all the Insights pages?

Copy link
Member

Choose a reason for hiding this comment

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

fair enough

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Is there anything else I should do to get your quality approval? 👀

Copy link
Member

Choose a reason for hiding this comment

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

Not going to treat this as a blocking comment, but does the whole data need to be made a prop, or would it be safe just to have the component receive the users, since that's the only part the component seems like it would ever care about?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, it could but since its goal is to display the UserActivityInsightsResponse data I think it is ok to use it.

I think it is just easier to maintain and extend using the context object than passing prop by prop.

Copy link
Member

@Parkreiner Parkreiner left a comment

Choose a reason for hiding this comment

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

Looks good! Just left one small comment about how much one of the components should be aware of the data coming into it

</PanelTitle>
</PanelHeader>
<PanelContent>
{!data && <Loader sx={{ height: "100%" }} />}
Copy link
Member

Choose a reason for hiding this comment

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

Not going to treat this as a blocking comment, but does the whole data need to be made a prop, or would it be safe just to have the component receive the users, since that's the only part the component seems like it would ever care about?

@BrunoQuaresma BrunoQuaresma merged commit 6651aff into main Oct 4, 2023
@BrunoQuaresma BrunoQuaresma deleted the bq/show-user-activity branch October 4, 2023 14:37
@github-actions github-actions bot locked and limited conversation to collaborators Oct 4, 2023
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.

Template Insights: Top users list & session length to API
3 participants