Skip to content
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

add notification to admin dash if no stats generated yet #346

Merged
merged 4 commits into from
Dec 17, 2024

Conversation

vincanger
Copy link
Collaborator

@vincanger vincanger commented Dec 16, 2024

Description

Fixes #344 and removes error being thrown if no stats have been calculated and generated yet by the dailyStatsJob

Contributor Checklist

Make sure to do the following steps if they are applicable to your PR:

@vincanger vincanger requested a review from infomiho December 16, 2024 15:38
Copy link
Collaborator

@infomiho infomiho left a comment

Choose a reason for hiding this comment

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

Looking good! Left one comment.

Also, I'd hide the banner below until we have the stats:

Screenshot 2024-12-17 at 16 29 10

<TotalSignupsCard dailyStats={stats?.dailyStats} isLoading={isLoading} />
</div>
<div className='relative'>
<div className={`${!stats ? 'opacity-25' : ''}`}>
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd recommend using clsx (and maybe twMerge alongside it) so you can write this more easily:

<div className={cn({
  opacity-25: !stats,
})}>
...
</div>

This enables you to compose styles in an object and then the clsx gives you the final string. It's easier to write and read IMHO.

ShadCN packs it into a cn helper fn:

import { clsx, type ClassValue } from "clsx"
import { twMerge } from "tailwind-merge"

export function cn(...inputs: ClassValue[]) {
  return twMerge(clsx(inputs))
}

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

we actually alread have the cn helper function i just forgot to use it :)

@vincanger vincanger merged commit a0de7e9 into main Dec 17, 2024
1 check passed
@infomiho
Copy link
Collaborator

@vincanger you merged, but did you hide the Stripe test purchases banner?

@vincanger
Copy link
Collaborator Author

@infomiho no I just left it becuase its only for opensaas.sh and for that we have a lot stats data in the db

@infomiho
Copy link
Collaborator

@vincanger Makes sense, but it still feels little dirty not to hide it in case we don't have any data. I know we have it, but you know, to make this as nice as possible of an implementation :)

@vincanger
Copy link
Collaborator Author

ok I fixed it and pushed to main

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

Successfully merging this pull request may close these issues.

getDailyStats HttpError being thrown with invalid status code
2 participants