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

frontend: Add CronJobListView story to test cronstrue dependency #2812

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

adwait-godbole
Copy link
Contributor

@adwait-godbole adwait-godbole commented Jan 30, 2025

Fixes #2374

Screencast.from.2025-01-30.19-12-59.webm

@illume can you please review this PR. Thanks.

@dosubot dosubot bot added the size:M This PR changes 30-99 lines, ignoring generated files. label Jan 30, 2025
@sniok
Copy link
Contributor

sniok commented Jan 30, 2025

Hi, thanks for opening a PR!
Before we do a full review could make sure your PR follows the guidelines? https://headlamp.dev/docs/latest/contributing#submitting-a-pull-request-pr

Make sure the tests are passing and you use atomic commits

@adwait-godbole adwait-godbole changed the title Add CronJob story frontend: Add CronJob story Jan 30, 2025
@adwait-godbole adwait-godbole changed the title frontend: Add CronJob story frontend: Add CronJobListView story Jan 30, 2025
@adwait-godbole adwait-godbole changed the title frontend: Add CronJobListView story frontend: Add CronJobListView story to test cronstrue dependency Jan 30, 2025
@adwait-godbole
Copy link
Contributor Author

Hi @sniok I have updated the PR title and description and now following atomic commits as per the guidelines. Also I have made sure that the tests and linting are passing. Please let me know if anything else. Thanks.

@sniok
Copy link
Contributor

sniok commented Jan 30, 2025

Thanks for updating it
After running npm run test in frontend folder a snapshot for the story is generated which should be added to the commit

other than that the PR looks good!

@adwait-godbole
Copy link
Contributor Author

After running npm run test in frontend folder a snapshot for the story is generated which should be added to the commit

Ohh. Yes. I did see it getting generated but I thought it wasn't necessary to be attached over here but anyways I will do it. Thanks.

@dosubot dosubot bot added size:XXL This PR changes 1000+ lines, ignoring generated files. and removed size:M This PR changes 30-99 lines, ignoring generated files. labels Jan 30, 2025
@adwait-godbole
Copy link
Contributor Author

Hi @sniok. Can you also re-review this one? Thanks.

@adwait-godbole
Copy link
Contributor Author

Also, do you think we should write e2e tests for this? or just this storybook is fine? The original issue mentioned either one is fine. So just confirming once from your side.

@adwait-godbole
Copy link
Contributor Author

adwait-godbole commented Feb 3, 2025

or I could open one more PR for the e2e tests. That's fine with me.

@sniok
Copy link
Contributor

sniok commented Feb 3, 2025

e2e tests are separate, so just storybook is fine
Could you update the commit message to start with frontend: ?

@adwait-godbole
Copy link
Contributor Author

Could you update the commit message to start with frontend: ?

Done!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size:XXL This PR changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add test for crontstrue dependency in cronjob/List.tsx
2 participants