added disableSnapshot param to batch files with 'Pages' in title #30718
Conversation
There was a problem hiding this comment.
I think a couple unwanted changes snuck in with this commit, which are causing the CI checks to fail! Don't worry, we've all done that before. 😛 And it's a great opportunity to practice more git! Let's see if we can drop those auto-generated schema files and the test changes we were making for the DiffHunk stuff.
…removed previous unused changes to diffHunk
|
Codenotify: Notifying subscribers in CODENOTIFY files for diff 5d7cf9d...ef21466.
|
.gitignore
Outdated
| @@ -106,6 +106,7 @@ coverage/ | |||
| out/ | |||
| client/shared/src/schema.ts | |||
| client/shared/src/schema/* | |||
| client/shared/src/graphql/schema.ts | |||
There was a problem hiding this comment.
Ideally, this should be in a separate PR, but if someone frontendy (like Kelli or Erik) is OK with this, then I am too.
There was a problem hiding this comment.
Yeah in general, things that aren't concerned with the main purpose of that PR should be done separately.
For this particular case, I wonder if it's actually needed at all, this file was moved to a different place and should not exist on disk anymore, and if so should just be removed. When we just gitignore it, it could stay around and might cause confusion when this file is accidentally loaded into the TS project? No strong opinion though.
There was a problem hiding this comment.
ahh yes, thanks for the heads up on this. I'll make note of that for good practice.
I added that to gitignore because i was having conflicts with that file showing up when i was trying to push the changes on friday. I'll remove it now
There was a problem hiding this comment.
LGTM, thanks for doing this :)
One small thought, I think we should file a follow-up ticket (or keep the original one open) to re-evaluate how well our components are covered in snapshot tests now. This is a good change to act quickly on our exploding screenshot number, but I think ideally we'd be evaluating some of the individual stories as well, looking where we can combine stories into a single chromatic snapshot perhaps.
.gitignore
Outdated
| @@ -106,6 +106,7 @@ coverage/ | |||
| out/ | |||
| client/shared/src/schema.ts | |||
| client/shared/src/schema/* | |||
| client/shared/src/graphql/schema.ts | |||
There was a problem hiding this comment.
Yeah in general, things that aren't concerned with the main purpose of that PR should be done separately.
For this particular case, I wonder if it's actually needed at all, this file was moved to a different place and should not exist on disk anymore, and if so should just be removed. When we just gitignore it, it could stay around and might cause confusion when this file is accidentally loaded into the TS project? No strong opinion though.
|
@eseliger yea good point, I'll close the original but create a follow up ticket tomorrow to start adressing this |
) * added disableSnapshot param to batch files with 'Pages' in title * added disableSnapshot param to batch files with 'Pages' in title and removed previous unused changes to diffHunk * Delete settings.schema.d.ts * Delete json-schema-draft-07.schema.d.ts * Delete batch_spec.schema.d.ts * Delete site.schema.d.ts * Delete schema.ts * removed extra spacing * removed all changes from diff files and removed viewport specs that were not there before * removed schema from git ignore
Closes #30337
As per Issue #30337, these changes are to add the
disableSnapshot: falseto batch change story parameters that we still wish to have snapshotting on