-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
feat(cdp): improve testing interface #27054
Conversation
Size Change: 0 B Total Size: 1.11 MB ℹ️ View Unchanged
|
📸 UI snapshots have been updated2 snapshot changes in total. 0 added, 2 modified, 0 deleted:
Triggered by this commit. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd say this is more than good enough for an incremental change. If we add some tracking and see if anyone uses it I think thats the way to know whether to invest harder
size="small" | ||
icon={<IconX />} | ||
onClick={() => deleteSavedGlobals(index)} | ||
tooltip="Delete this saved set of globals" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I'd opt for "save test data" or something as globals sounds a bit "internal"
|
||
savedGlobals: [ | ||
[] as { name: string; globals: HogFunctionInvocationGlobals }[], | ||
{ persist: true, prefix: `${getCurrentTeamId()}__` }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Personally I think this is fine. I'd say, lets add some tracking to this. If we find a lot of usage then we can consider a more robust persistence?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added a bunch of data-attr
-s all over the place and called these things "test data" everywhere. I guess good enough?
📸 UI snapshots have been updated1 snapshot changes in total. 0 added, 1 modified, 0 deleted:
Triggered by this commit. |
📸 UI snapshots have been updated1 snapshot changes in total. 0 added, 1 modified, 0 deleted:
Triggered by this commit. |
Changes
How did you test this code?
Played around in the UI.
Persisting into localstorage isn't obvious though --> as a user I'd expect this to live in a database somewhere. Should we put this somewhere, or make the persistance class more clear in the UX?